| Summary: | Add separate permission for Merging Patrons | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Rebecca Coert <rcoert> |
| Component: | Patrons | Assignee: | Brendan Lawlor <blawlor> |
| Status: | Pushed to oldoldoldstable --- | QA Contact: | David Cook <dcook> |
| Severity: | critical | ||
| Priority: | P5 - low | CC: | blawlor, chanel.wheeler, dcook, didier.gautheron, eric, fridolin.somers, gmcharlt, jesse, jonathan.druart, katrin.fischer, kyle, laurent.ducos, lisette, lucas, m.de.rooy, martin.renvoize, mspinney, nick, phil, philippe.blouin, tomascohen, wainuiwitikapark |
| Version: | unspecified | ||
| Hardware: | All | ||
| OS: | All | ||
| 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
Bug 35830: Add merge_borrowers subpermission Bug 35830: Add merge_borrowers subpermission Bug 35830: Add merge_borrowers subpermission Bug 35830: Add merge_borrowers subpermission Bug 35830: [25.05.x] Add merge_borrowers subpermission |
||
|
Description
Rebecca Coert
2024-01-17 16:13:49 UTC
+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 |