Bug 38040

Summary: IndependentBranches doesn't prevent editing other libraries' holds
Product: Koha Reporter: Caroline Cyr La Rose <caroline.cyr-la-rose>
Component: Hold requestsAssignee: Hammat wele <hammat.wele>
Status: In Discussion --- QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: david, dcook, gmcharlt, jonathan.druart, lin-wei.li, m.de.rooy, nick, philippe.blouin
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35434
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39433
GIT URL: Change sponsored?: ---
Patch complexity: Small patch Documentation contact:
Documentation submission: Text to go in the release notes:
This fixes holds so that staff patrons can only edit or delete holds for their library when: - IndependentBranches = "Yes" - IndependentBranchesPatronModifications = "Yes" - IndependentBranchesTransfers = "Yes" - canreservefromotherbranches = "Don't allow (with independent branches)". Previously, any staff member from any library could edit or delete the holds for patrons from other libraries. Only superlibrarians should be able to edit or delete all holds.
Version(s) released in:
Circulation function:
Attachments: Bug 38040: Disabled tables element for staff
Bug 38040: Prevent editing other libraries' holds when IndependentBranches is enabled
Bug 38040: Prevent editing other libraries' holds when IndependentBranches is enabled
Bug 38040: Prevent editing other libraries' holds when IndependentBranches is enabled
Bug 38040: Prevent editing other libraries' holds when IndependentBranches is enabled
Bug 38040: Light reformating
Bug 38040: Prevent editing other libraries' holds when IndependentBranches is enabled
Bug 38040: Adding server-side verification
Bug 38040: Prevent editing other libraries' holds when IndependentBranches is enabled
Bug 38040: Adding server-side verification
Bug 38040: Prevent editing other libraries' holds when IndependentBranches is enabled
Bug 38040: Adding server-side verification
Bug 38040: [alternate] Restrict hold management if cannot see patron data
Bug 38040: Preventing editing other libraries' holds with IndependentBranches enabled
Bug 38040: DB Update: Adding IndependentBranchesHolds syspref
Bug 38040: Preventing editing other libraries' holds with IndependentBranches enabled
Bug 38040: DB Update - Adding IndependentBranchesHolds syspref
Bug 38040: [alternate] Restrict hold management if cannot see patron data
Bug 38040: Preventing editing other libraries' holds with IndependentBranches enabled
Bug 38040: DB Update - Adding IndependentBranchesHolds syspref
Bug 38040: DB Update - Adding IndependentBranchesHolds syspref
Bug 38040: [alternate] Restrict hold management if cannot see patron data
Bug 38040: Preventing editing other libraries' holds with IndependentBranches enabled
Bug 38040: DB Update - Adding IndependentBranchesHolds syspref

Description Caroline Cyr La Rose 2024-09-30 16:48:08 UTC
When IndependentBranches is set, any staff member from any library can see and edit the holds from patrons from other libraries.


1. Set IndependentBranches, IndependentBranchesPatronModifications, and IndependentBranchesTransfers to 'Yes'

2. Set canreservefromotherbranches to "Don't allow (with independent branches)"

3. Give limited permissions to a staff patron
   - circulate
   - catalogue
   - delete_borrowers
   - edit_borrowers
   - list_borrowers
   - send_messages_to_borrowers
   - reserveforothers
   - updatecharges

4. As the superlibrarian, place several holds on a record with items from various branches

5. Log in as the restricted staff

6. View the holds for the record
   --> Some will say "A patron from library XX" instead of the name -- OK

7. Click the trash bin icon to cancel a hold from another library
   --> Hold is cancelled -- NOT OK

I feel like it should only be superlibrarians who can edit all holds, even those from other libraries.
Comment 1 Sukhmandeep 2024-10-25 16:38:30 UTC Comment hidden (obsolete)
Comment 2 David Nind 2025-01-01 20:48:59 UTC Comment hidden (obsolete)
Comment 3 David Nind 2025-01-01 20:58:44 UTC
I updated the assignee, edited the commit message, and added a release note.
Comment 4 Jonathan Druart 2025-01-23 11:23:12 UTC
1. is_superlibrarian is already available from every templates
Use logged_in_user.is_superlibrarian or CAN_user_superlibrarian

2. Same for the logged in library, use Branches.GetLoggedInBranchcode

3. Useless indentation change in request.pl

So basically you don't need any changes from the controller.

Is this enforce server-side?
Comment 5 William Lavoie 2025-01-27 19:46:15 UTC Comment hidden (obsolete)
Comment 6 William Lavoie 2025-01-27 19:51:14 UTC Comment hidden (obsolete)
Comment 7 William Lavoie 2025-01-28 19:51:06 UTC Comment hidden (obsolete)
Comment 8 David Nind 2025-02-02 18:49:32 UTC
I've retested, but for step 8 I can still cancel holds made by other libraries.

Please also amend the commit message so that the description describes the problem - similar to comment #2. See the commit message guidelines https://wiki.koha-community.org/wiki/Commit_messages

(Please don't put comments in the commit message like "Rebased" - these can be done as normal bug comments.)
Comment 9 William Lavoie 2025-02-03 17:35:52 UTC Comment hidden (obsolete)
Comment 10 William Lavoie 2025-02-03 17:51:44 UTC
Hi David, 

Thank you for the feedback, I had accidentally reverted the changes from previous patches. Hopefully this should work now. 

This patch implements the corrections from comment 4.
Comment 11 David Nind 2025-02-03 23:09:58 UTC Comment hidden (obsolete)
Comment 12 David Nind 2025-02-03 23:12:05 UTC
(In reply to William Lavoie from comment #10)
> Hi David, 
> 
> Thank you for the feedback, I had accidentally reverted the changes from
> previous patches. Hopefully this should work now. 
> 
> This patch implements the corrections from comment 4.

Thanks William!

I've squashed the patches (not sure whether I should have, but I think that is OK in this instance).

I've changed the assignee as well.

Good luck with the QA steps!

David
Comment 13 Nick Clemens (kidclamp) 2025-02-07 17:05:31 UTC
This patch only deals with the template, I can easily edit the html, remove the disabled class, and still cancel the hold. A check needs to be added into the perl scripts or the modules.

It also affects all non-superlibrarians, regardless of independentbranches being enabled/disabled, we need to check that preference first.

I think maybe we need an additional permission for reserves here - which says whether you can affect all holds or not. Taking away the ability to modify/cancel holds would be a big change in behavior that I am not sure all libraries using independent branches would agree to.

The trend has been to move parts of Independent Branches to more granular system preferences ( IndependentBranchesPatronModifications ,  IndependentBranchesTransfers ) so maybe we need 'IndependentBranchesReserves'?

This current code could be adapted to work in IntranerUserJS if it is sufficient for your library, happy to help with that or any of the changes above if I can
Comment 14 William Lavoie 2025-02-13 14:53:55 UTC Comment hidden (obsolete)
Comment 15 William Lavoie 2025-02-13 14:58:12 UTC Comment hidden (obsolete)
Comment 16 William Lavoie 2025-02-13 14:58:14 UTC Comment hidden (obsolete)
Comment 17 William Lavoie 2025-02-13 15:00:06 UTC
Hi Owen,
Thank you for the feedback, I have added checks in the perl scripts for deletion as well as the other operations (changing priority, adding an expiration date, batch deletions, etc). After a discussion with Caroline, we agreed with your solution to add a system preference 'IndependentBranchesHolds' (the Koha terminology favors 'Hold' over 'Reserve'). I have added an atomic update.

Additionally, in order to allow users to change priority within their own library, I changed the way holds are displayed so that for non-superlibrarians, when 'IndependentBranchesHolds' is set to 'Yes', the holds are displayed by library, similarly to having 'HoldsSplitQueue' set to 'branch'.
Comment 18 William Lavoie 2025-02-13 15:02:54 UTC
(In reply to William Lavoie from comment #17)
My apologies, I meant to address Nick.
Comment 19 William Lavoie 2025-03-11 20:09:11 UTC Comment hidden (obsolete)
Comment 20 William Lavoie 2025-03-11 20:09:14 UTC Comment hidden (obsolete)
Comment 21 David Cook 2025-03-24 05:27:26 UTC
Comment on attachment 179186 [details] [review]
Bug 38040: Prevent editing other libraries' holds when IndependentBranches is enabled

Review of attachment 179186 [details] [review]:
-----------------------------------------------------------------

::: reserve/request.pl
@@ +39,4 @@
>  use C4::Serials     qw( CountSubscriptionFromBiblionumber );
>  use C4::Circulation qw( _GetCircControlBranch GetBranchItemRule );
>  use Koha::DateUtils qw( dt_from_string );
> +<<<<<<< HEAD

This patch contains a git merge error
Comment 22 David Cook 2025-03-24 05:28:06 UTC
There's a git merge error in the second patch. Can you fix that and squash the patches together? (Then perhaps split out the DB update into a separate patch?)
Comment 23 David Cook 2025-03-24 05:34:48 UTC
Thanks for your work to-date on this one. I need this fix soon for one of my libraries, as I think it's the last piece of the puzzle of IndependentBranches/LibraryGroups/heaps of my own fixes to get Koha usable for this particular set of libraries...
Comment 24 David Cook 2025-03-24 05:35:41 UTC
(In reply to David Cook from comment #23)
> Thanks for your work to-date on this one. I need this fix soon for one of my
> libraries, as I think it's the last piece of the puzzle of
> IndependentBranches/LibraryGroups/heaps of my own fixes to get Koha usable
> for this particular set of libraries...

What I mean is... I can dedicate time to working with inLibro on this.
Comment 25 David Cook 2025-03-24 05:38:20 UTC
So I Failed QA on the 2nd patch, and now I'm marking "Patch doesn't apply" on the 1st patch too.

error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt).
error: could not build fake ancestor

I've fetched from the Koha upstream, so that suggests that this patch has been based off a local commit that doesn't exist in the Koha upstream.

Please rebase.
Comment 26 David Cook 2025-03-24 05:45:03 UTC
Inspecting the code more closely and looks like you've got your patches out of order as well. 

Btw in installer/data/mysql/mandatory/sysprefs.sql the syspref needs to be added in alphabetical order by variable name.
Comment 27 David Cook 2025-03-24 05:52:20 UTC
Lots of unnecessary white space changes as well. 

Noticing a lot of spaces around the method operator -> as well which might not get caught by a tidy but is atypical for Koha.
Comment 28 David Cook 2025-03-25 01:06:43 UTC
Since this one is quite important for me, I'm going to try to resurrect it...
Comment 29 David Cook 2025-03-25 06:24:43 UTC
I was busy today, but taking a quick look at the code.

Needed to add the following to request.tt as it wasn't disabling the pickup location dropdown:

$(this).find('.pickup_location_dropdown').prop('disabled', true);

--

It looks like these patches don't take into account modrequest.pl? That script is invoked by the "Update hold(s)" button. (Holds/reserves code truly is a nightmare, eh?)

--

I am tempted to take the frontend shortcut for my current project, since validating server-side is a minefield. 

I'll think on this more tomorrow...
Comment 30 David Cook 2025-03-25 23:58:49 UTC
So... I'm a bit confused here. 

I've followed the test plan and I've reviewed the code, and it seems to me that the text "A patron from library Centerville" comes from the Library Groups feature and not IndependentBranches. Because I can't get  "A patron from library Centerville" to appear in main following the test plan, and when I look at the code that makes sense, because it's a Library Groups feature. 

As a result, it seems to me that the solution should actually be with Library Groups and not IndependentBranches.
Comment 31 David Cook 2025-03-26 00:29:17 UTC
(In reply to David Cook from comment #30)
> As a result, it seems to me that the solution should actually be with
> Library Groups and not IndependentBranches.

Oh wait a minute...the text "A patron from library Centerville" shows for both features. I was looking for IndependentBranches in Koha/Patron.pm but now I see C4::Context::only_my_library. I made a mistake in my testing as well.

Koha::Patron->libraries_where_can_see_things is pretty suboptimal for Library Groups... iterating through every group and subgroup to find the ones that apply to a borrower at the code level rather than SQL...

It would be interesting to have a "Limit hold management by group" for Library Groups though...
Comment 32 David Cook 2025-03-26 00:46:25 UTC
Since the holds in tmpl/intranet-tmpl/prog/en/includes/holds_table.inc are actually hashrefs from reserve/request.pl we can actually do the permission check in the controller, and then we just look for the flag in the template/view so that we can toggle the display accordingly.

Something like "$logged_in_user->can_manage_hold($hold->patron)" would be good, because it could apply to both IndependentBranches and Library Groups.

Plus we can then use it for validating the actions/ops server-side as well. 

The only question that remains then is how to gracefully handle errors server-side. Although really I think something like pre-validating before the action/op and throwing a 403 is not a bad idea. Simple and effective.
Comment 33 David Cook 2025-03-26 01:30:27 UTC Comment hidden (obsolete)
Comment 34 David Cook 2025-03-26 01:33:59 UTC
I've uploaded an alternate patch, which hides the hold management controls at the template level if you can't see the patron data (via either IndependentBranches or Library Groups).

This serves my immediate purposes, but let me know what you think at the community level, and we can keep working on the feature (ie validating requests to request.pl, modrequest.pl, etc.)

--

Note that even with the controls hidden, if your logged in patron has permission to change priority, they can still change their own holds relative to holds from other branches.
Comment 35 David Cook 2025-03-26 01:39:40 UTC
Btw, if you view my patch with "git show -w", it's much less scary than the Splinter Review. 

These are the stats without the whitespace change:
2 files changed, 26 insertions(+), 3 deletions(-)
Comment 36 William Lavoie 2025-04-03 13:47:28 UTC Comment hidden (obsolete)
Comment 37 William Lavoie 2025-04-03 13:47:30 UTC Comment hidden (obsolete)
Comment 38 William Lavoie 2025-04-03 14:44:34 UTC Comment hidden (obsolete)
Comment 39 William Lavoie 2025-04-03 14:44:37 UTC Comment hidden (obsolete)
Comment 40 William Lavoie 2025-04-03 14:54:36 UTC
Hello David, 
I appreciate the feedback and I tried to address everything you brought up in these patches. I merged the front-end and back-end patches into a single one and separated the DB update, I removed white spaces, I put the syspref in alphabetical order and I added server-side validation in modrequest.pl. 

If the alternate patch is preferred then I can help with the back-end modifications.
Comment 41 William Lavoie 2025-04-03 14:55:09 UTC
Test plan:
1. Set IndependentBranches, IndependentBranchesPatronModifications,
   IndependentBranchesTransfers and IndependentBranchesHolds to 'Yes'.
2. Set canreservefromotherbranches to "Don't allow (with
   independent branches)".
3. Give limited permissions to a staff patron
   - circulate
   - catalogue
   - delete_borrowers
   - edit_borrowers
   - list_borrowers
   - send_messages_to_borrowers
   - reserveforothers
   - updatecharges
4. As the superlibrarian, place several holds on a record with
   items from various branches.
5. Log in as the restricted staff
6. View the holds for the record
   --> Some will say "A patron from library XX" instead
   of the name -- OK
7. Click the trash bin icon to cancel a hold from another library
   --> Hold is cancelled -- NOT OK
8. Apply the patch and update the database
9. Validate that step 7 is not possible now and that holds from the staff's library can be modified or deleted.
Comment 42 Magnus Enger 2025-04-14 07:40:19 UTC
Created attachment 180898 [details] [review]
Bug 38040: [alternate] Restrict hold management if cannot see patron data

This patch leverages the Koha::Patron->can_see_patron_infos() function
to determine whether or not the UI will display hold management functions. If
the logged in user cannot see the patron info, it seems to follow that they should not
be able to manage the hold either.

NOTE: This should work for both IndependentBranches and Library Group functionality.

This is only the front-end modification.

To be coded: a back-end modification that throws a 403
for stateful actions to holds the logged in user is not allowed to manage.

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 43 Magnus Enger 2025-04-14 07:40:23 UTC
Created attachment 180899 [details] [review]
Bug 38040: Preventing editing other libraries' holds with IndependentBranches enabled

I added the verification to modrequest.pl as well.

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 44 Magnus Enger 2025-04-14 07:40:27 UTC
Created attachment 180900 [details] [review]
Bug 38040: DB Update - Adding IndependentBranchesHolds syspref

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 45 David Cook 2025-04-23 04:46:37 UTC
Magnus did you sign off both approaches?
Comment 46 Lin Wei 2025-05-14 13:23:17 UTC
Created attachment 182444 [details] [review]
Bug 38040: DB Update - Adding IndependentBranchesHolds syspref

Signed-off-by: Magnus Enger <magnus@libriotech.no>

Rebased to main.
Comment 47 Lin Wei 2025-05-14 13:31:02 UTC
Created attachment 182445 [details] [review]
Bug 38040: [alternate] Restrict hold management if cannot see patron data

This patch leverages the Koha::Patron->can_see_patron_infos() function
to determine whether or not the UI will display hold management functions. If
the logged in user cannot see the patron info, it seems to follow that they should not
be able to manage the hold either.

NOTE: This should work for both IndependentBranches and Library Group functionality.

This is only the front-end modification.

To be coded: a back-end modification that throws a 403
for stateful actions to holds the logged in user is not allowed to manage.

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 48 Lin Wei 2025-05-14 13:31:13 UTC
Created attachment 182446 [details] [review]
Bug 38040: Preventing editing other libraries' holds with IndependentBranches enabled

I added the verification to modrequest.pl as well.

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 49 Lin Wei 2025-05-14 13:31:17 UTC
Created attachment 182447 [details] [review]
Bug 38040: DB Update - Adding IndependentBranchesHolds syspref

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 50 Lin Wei 2025-05-14 13:32:09 UTC
Rebased
Comment 51 Marcel de Rooy 2025-05-30 07:54:42 UTC
(In reply to David Cook from comment #45)
> Magnus did you sign off both approaches?

This looks like a good question to be answered before starting QA. Could you please address it, Magnus ?
Thanks.

Changing status to reflect need for feedback.