Bug 35830 - Add separate permission for Merging Patrons
Summary: Add separate permission for Merging Patrons
Status: Pushed to oldoldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low critical
Assignee: Brendan Lawlor
QA Contact: David Cook
URL:
Keywords:
: 38754 (view as bug list)
Depends on:
Blocks:
 
Reported: 2024-01-17 16:13 UTC by Rebecca Coert
Modified: 2025-12-04 15:08 UTC (History)
22 users (show)

See Also:
GIT URL:
Initiative type: ---
Sponsorship status: ---
Comma delimited list of Sponsors:
Crowdfunding goal: 0
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.11.00,25.05.06,24.11.11,24.05.16,22.11.33
Circulation function:


Attachments
Bug 35830: Add merge_borrowers subpermission (5.91 KB, patch)
2025-09-09 14:13 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 35830: Add merge_borrowers subpermission (5.93 KB, patch)
2025-09-09 14:23 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 35830: Add merge_borrowers subpermission (6.25 KB, patch)
2025-09-29 16:08 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 35830: Add merge_borrowers subpermission (6.31 KB, patch)
2025-10-24 15:55 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 35830: Add merge_borrowers subpermission (6.36 KB, patch)
2025-10-31 01:41 UTC, David Cook
Details | Diff | Splinter Review
Bug 35830: [25.05.x] Add merge_borrowers subpermission (2.32 KB, patch)
2025-12-01 15:59 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Rebecca Coert 2024-01-17 16:13:49 UTC
Currently, the ability to Merge Patrons is lumped under the "Add, modify, and view patron information" (edit_borrowers).  Merging patron records is more powerful than the ability to add/modify/view a patron record. 

My request is to have this as a separate permission, similar to that of delete_borrowers.
Comment 1 Chanel Wheeler 2024-10-08 15:30:27 UTC
+1
Comment 2 Brendan Lawlor 2025-09-08 20:28:44 UTC
*** Bug 38754 has been marked as a duplicate of this bug. ***
Comment 3 Brendan Lawlor 2025-09-09 14:13:13 UTC
Created attachment 186301 [details] [review]
Bug 35830: Add merge_borrowers subpermission

Merging patrons only required edit_borrowers permission
but would result in a patron deletion.

This patch requires a new merge_borrowers subpermission

To test:
1. Give a patron the following permissions
    catalogue, edit_borrowers, list_borrowers
2. Log into the staff interface as the patron
3. Do a patron search and select 2 patrons
4. Click merge patron - success - a patron got deleted
5. Apply patch, updatedatabase and restart_all
6. Repeat steps 3 and 4 - you can't
7. Add the new permission for merge_borrowers
8. Repeat steps 3 and 4 - now you can
9. prove t/Koha/Auth/Patrons.t
Comment 4 Brendan Lawlor 2025-09-09 14:23:06 UTC
Created attachment 186302 [details] [review]
Bug 35830: Add merge_borrowers subpermission

Merging patrons only required edit_borrowers permission
but would result in a patron deletion.

This patch requires a new merge_borrowers subpermission

To test:
1. Give a patron the following permissions
    catalogue, edit_borrowers, list_borrowers
2. Log into the staff interface as the patron
3. Do a patron search and select 2 patrons
4. Click merge patron - success - a patron got deleted
5. Apply patch, updatedatabase and restart_all
6. Repeat steps 3 and 4 - you can't
7. Add the new permission for merge_borrowers
8. Repeat steps 3 and 4 - now you can
9. prove t/Koha/Auth/Patrons.t

Sponsored-by: CLAMS
Comment 5 Brendan Lawlor 2025-09-09 16:20:53 UTC
Setting to needs sign off for feedback.

I know I need to add the atomic update in a separate commit. 

I think this might also need to update the database to give the new merge_borrowers permission to everyone who currently has the delete_borrowers permission.

Or should it just give merge_borrowers to everyone with edit_borrowers because that's the current way it is?
Comment 6 Brendan Lawlor 2025-09-29 16:08:07 UTC
Created attachment 187060 [details] [review]
Bug 35830: Add merge_borrowers subpermission

Merging patrons only required edit_borrowers permission
but would result in a patron deletion.

This patch requires a new merge_borrowers subpermission

To test:
1. Give a patron the following permissions
    catalogue, edit_borrowers, list_borrowers
2. Log into the staff interface as the patron
3. Do a patron search and select 2 patrons
4. Click merge patron - success - a patron got deleted
5. Apply patch, updatedatabase and restart_all
6. Repeat steps 3 and 4 - you can't
7. Add the new permission for merge_borrowers
8. Repeat steps 3 and 4 - now you can
9. prove t/Koha/Auth/Patrons.t

Sponsored-by: CLAMS
Comment 7 Brendan Lawlor 2025-09-29 16:15:01 UTC
I added an insert statement to the atomic update to add the merge_borrowers permission for all users who currently have the edit_borrowers permission 

This makes the behavior for for existing installations consistent with the current functionality.
Comment 8 David Cook 2025-10-23 00:48:51 UTC
Something we should consider is how the new "merge_borrowers" permission intersects with the "staffaccess" permission.

On the face of it, it seems like you could bypass "staffaccess" by merging a higher privileged user into a lower privileged user. 

Thinking about this again... what's the original premise here? What makes merging patron records more powerful than "edit_borrowers"? Is it because of the permission setting?
Comment 9 David Cook 2025-10-23 00:52:55 UTC
(In reply to David Cook from comment #8)
> Something we should consider is how the new "merge_borrowers" permission
> intersects with the "staffaccess" permission.
> 
> On the face of it, it seems like you could bypass "staffaccess" by merging a
> higher privileged user into a lower privileged user. 
> 
> Thinking about this again... what's the original premise here? What makes
> merging patron records more powerful than "edit_borrowers"? Is it because of
> the permission setting?

On this note, it might be necessary to check the initiating user's permissions to see if they have "staffaccess" if they are going to do a patron merge that includes permissions... and either block if they don't or don't include permission merges or something else. 

But I haven't delved too deeply into this one. I don't have anyone that really uses patron merge, so it's not something I've really reviewed much before.
Comment 10 Brendan Lawlor 2025-10-23 15:01:03 UTC
(In reply to David Cook from comment #8)
> Something we should consider is how the new "merge_borrowers" permission
> intersects with the "staffaccess" permission.
> 
> On the face of it, it seems like you could bypass "staffaccess" by merging a
> higher privileged user into a lower privileged user. 

I just tested this and permissions are not merged into the patron that you keep. So that's good.

> Thinking about this again... what's the original premise here? What makes
> merging patron records more powerful than "edit_borrowers"? Is it because of
> the permission setting?

The original premise is that staff with edit_borrowers, but not delete_borrowers should not be able to merge patrons because the end result is a patron deletion. Currently library staff that should only be able to edit, but not delete patron records are overgranted with the power to merge. 

The merge function is actually even more powerful than delete_borrowers because it allows you to merge multiple patrons into one. If used maliciously a staff could do an empty patron search, select all and then merge all patrons into their record, essentially batch deleting all patrons but themselves.

This can be mitigated by using protected patrons, but could still be really bad. That's the reason I think merge should have it's own permission, or at the least it should require the delete_borrower permission.
Comment 11 David Cook 2025-10-24 03:10:16 UTC
(In reply to Brendan Lawlor from comment #10)
> This can be mitigated by using protected patrons, but could still be really
> bad. That's the reason I think merge should have it's own permission, or at
> the least it should require the delete_borrower permission.

Thanks for the explanation! I think I find that pretty compelling. Good reason to have a merge_borrower permission, and it doesn't really involve anything else, so it's pretty narrowly cast... sounds good to me.
Comment 12 Owen Leonard 2025-10-24 15:55:18 UTC
Created attachment 188417 [details] [review]
Bug 35830: Add merge_borrowers subpermission

Merging patrons only required edit_borrowers permission
but would result in a patron deletion.

This patch requires a new merge_borrowers subpermission

To test:
1. Give a patron the following permissions
    catalogue, edit_borrowers, list_borrowers
2. Log into the staff interface as the patron
3. Do a patron search and select 2 patrons
4. Click merge patron - success - a patron got deleted
5. Apply patch, updatedatabase and restart_all
6. Repeat steps 3 and 4 - you can't
7. Add the new permission for merge_borrowers
8. Repeat steps 3 and 4 - now you can
9. prove t/Koha/Auth/Patrons.t

Sponsored-by: CLAMS
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 13 David Cook 2025-10-31 01:40:51 UTC
The test plan isn't 100% accurate because of the automatic addition of "merge_borrowers" when you have "edit_borrowers", but that's OK. I could figure it out easy enough.

Overall, I think this works great. Good one, Brendan!
Comment 14 David Cook 2025-10-31 01:41:36 UTC
Created attachment 188723 [details] [review]
Bug 35830: Add merge_borrowers subpermission

Merging patrons only required edit_borrowers permission
but would result in a patron deletion.

This patch requires a new merge_borrowers subpermission

To test:
1. Give a patron the following permissions
    catalogue, edit_borrowers, list_borrowers
2. Log into the staff interface as the patron
3. Do a patron search and select 2 patrons
4. Click merge patron - success - a patron got deleted
5. Apply patch, updatedatabase and restart_all
6. Repeat steps 3 and 4 - you can't
7. Add the new permission for merge_borrowers
8. Repeat steps 3 and 4 - now you can
9. prove t/Koha/Auth/Patrons.t

Sponsored-by: CLAMS
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 15 Lucas Gass (lukeg) 2025-12-01 15:59:34 UTC
Created attachment 190058 [details] [review]
Bug 35830: [25.05.x] Add merge_borrowers subpermission

Merging patrons only required edit_borrowers permission
but would result in a patron deletion.

This patch requires a new merge_borrowers subpermission

To test:
1. Give a patron the following permissions
    catalogue, edit_borrowers, list_borrowers
2. Log into the staff interface as the patron
3. Do a patron search and select 2 patrons
4. Click merge patron - success - a patron got deleted
5. Apply patch, updatedatabase and restart_all
6. Repeat steps 3 and 4 - you can't
7. Add the new permission for merge_borrowers
8. Repeat steps 3 and 4 - now you can
9. prove t/Koha/Auth/Patrons.t

Sponsored-by: CLAMS
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 16 Lucas Gass (lukeg) 2025-12-01 16:03:18 UTC
The 25.05 patch should also work for 24.11 and 24.05
Comment 17 Lucas Gass (lukeg) 2025-12-01 16:04:21 UTC
The 25.05 patch should work for 22.11 as well!
Comment 18 Jesse Maseto 2025-12-01 16:35:01 UTC
Thank you Lucas. This will be in 24.05.16
Comment 19 Fridolin Somers 2025-12-02 10:20:18 UTC
Oh crap.

Looks like this is pushed to public repo (main and 25.05.x) :
https://git.koha-community.org/Koha-community/Koha/commits/branch/main/search?q=35830&all=

But in my opinion this is a minor security level.
We could release it as a non-security issue.
Comment 20 Wainui Witika-Park 2025-12-03 02:11:18 UTC
I have applied this to 22.11.x-security for 22.11.33 however I just wanted to note:

- there is no list_borrowers permission - but I don't think that matters

- there is no Koha::Installer::Output module so I have to take out the "use Koha::Installer::Output qw(say_warning say_success say_info);" line in Bug_35830_add_merge_borrowers_permission.pl

- I can't find a t/Koha/Auth/Patrons.t file
Comment 21 Fridolin Somers 2025-12-04 09:38:10 UTC
Since this is pushed to public repo in main and 25.05.x
I push to 24.11.x for 24.11.11

Discussed on Mattermost with Lucas.
Comment 22 Lucas Gass (lukeg) 2025-12-04 14:22:42 UTC
Pushed to main for 25.11.00