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.
+1
*** Bug 38754 has been marked as a duplicate of this bug. ***
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
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
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?
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
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.
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?
(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.
(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.
(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.
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>
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!
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>
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>
The 25.05 patch should also work for 24.11 and 24.05
The 25.05 patch should work for 22.11 as well!
Thank you Lucas. This will be in 24.05.16
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.
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
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.
Pushed to main for 25.11.00
Might be only mine specific data configuration, but on my dataset atomic update crashed: ``` Upgrade to 25.06.00.028 [00:37:40]: Bug 35830 - Add permission borrowers:merge_borrowers Added new permission 'merge_borrowers' ERROR: {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Duplicate entry '281100-4-merge_borrowers' for key 'PRIMARY' at /usr/share/koha/lib/C4/Installer.pm line 825 ``` writing this urgently here, because if this is potential atomic update crash, it will lock up Kohas for people around the world. But this might be false alarm. Investigating: I will research for a workday and give my findings, how this is critical.
Indeed. We have: ``` q{INSERT INTO user_permissions (borrowernumber, module_bit, code) SELECT borrowernumber, module_bit, 'merge_borrowers' FROM user_permissions where module_bit = 4 and code = 'edit_borrowers';} ``` what is somewhat-non-idempotent, BUT it will luckily succeed if someone missed 25.05.06 update and went to 25.11.00, but if someone have 25.05.06 then 25.11.00 probably will fail: ``` $> git checkout community/25.05.x && fgrep "Added new permission 'merge_borrowers'" installer/data/mysql/db_revs/*.pl Previous HEAD position was 3f2987eb19f Koha 25.11.00 is here! HEAD is now at aa6597d23ec Update release notes for 25.05.06 release installer/data/mysql/db_revs/250505004.pl: say $out "Added new permission 'merge_borrowers'"; $> git checkout community/25.11.x && fgrep "Added new permission 'merge_borrowers'" installer/data/mysql/db_revs/*.pl Previous HEAD position was aa6597d23ec Update release notes for 25.05.06 release HEAD is now at 3f2987eb19f Koha 25.11.00 is here! installer/data/mysql/db_revs/250600028.pl: say $out "Added new permission 'merge_borrowers'"; ``` because it will try to non-ignore insert for already existing values, thus PRIMARY key fails.
.. I did fixed my server with patching this SQL INSERT as "IGNORE": ``` q{INSERT IGNORE INTO user_permissions (borrowernumber, module_bit, code) SELECT borrowernumber, module_bit, 'merge_borrowers' FROM user_permissions where module_bit = 4 and code = 'edit_borrowers';} ``` and re-run `apt-get upgrade`, all passed. Not sure this is the cleanest way for now, but this might be workarounder for other non-deep-Koha people who come here from the websearch one day too.
This is not data-specific. Any site that has already applied the 25.05.06 atomic update for Bug 35830 and then runs 25.06.00.028 (25.11.00), or has applied the 24.11.11 update and then upgrades to 25.05 or 25.11, will hit this duplicate-key error and get stuck in maintenance mode until it is manually patched. Changing above statement to use `INSERT IGNORE ...` makes the atomic update idempotent and fixes the issue. If `INSERT IGNORE ...` is considered enough, the same change should be backported to all older versions that contain this atomic update to avoid overlapping executions of the same insert on older upgrades sequences.
I think the missing bit is that the database update cannot be run multiple times at the moment, which we need to fix. Correct?
Yes, exactly - the missing bit was that the atomic DB update was not idempotent, and the same non-idempotent `INSERT` ended up being shipped in multiple branches and then overlaid: once it successfully runs on an older version, a later upgrade that runs the same `INSERT` again in a newer branch hits the duplicate key and crashes update database in the middle.
... Lucas (@lukeg) has now added fixes for all affected branches in Bug 41421 by changing the `user_permissions` statement to `INSERT IGNORE` in each branch, so that re-running the update (or running it twice across different branches) simply skips rows that already exist instead of throwing an error. I’m OK with the `INSERT IGNORE` approach here — it makes this update properly idempotent and unblocks all the upgrade paths. Minor downsides I can see is that: - if there ever were some unexpected data issues or duplicates in `user_permissions`, they would now be silently skipped instead of failing loudly, and - `updatedatabase` will still print “Added 'merge_borrowers' permission to existing users with 'edit_borrowers'” even on subsequent runs where no new rows are actually inserted. For this very narrow case I think that trade-off is acceptable, especially given how important it is that we deliver it asap for installs on a broken upgrade path. From a packager point of view (e.g. .deb builds) it’s great that this has now been fixed in all relevant branches as quickly as it was, and I don't know how many shipped and potential overlays around the world, but I am just noting this, not a question.