Bug 39848 - Users without edit_catalogue permission can delete the record if no items remain from the batch item deletion tool
Summary: Users without edit_catalogue permission can delete the record if no items rem...
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Lucas Gass (lukeg)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2025-05-06 20:55 UTC by Catrina Berka
Modified: 2025-05-19 16:37 UTC (History)
4 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.05.00
Circulation function:


Attachments
Bug 39848: Restrict record deletion to staff who have edit_catalogue permission (2.31 KB, patch)
2025-05-06 22:22 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 39848: Restrict record deletion to staff who have edit_catalogue permission (2.36 KB, patch)
2025-05-07 13:59 UTC, Roman Dolny
Details | Diff | Splinter Review
Bug 39848: Restrict record deletion to staff who have edit_catalogue permission (2.42 KB, patch)
2025-05-08 12:34 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Catrina Berka 2025-05-06 20:55:13 UTC
Users without the edit_catalogue permission can delete the record if no items remain from the batch item deletion tool. 

To test, 
1. Create a user with the following permissions: edit_any_item, edit_items, items_batchdel, items_batchmod. 
2. When logged in as that user, use the batch item deletion tool to delete a few items. 
3. Select “Delete records if no items remain” and “Delete selected items.”
When the job has processed, notice that X item(s) deleted. X record(s) deleted. 

The record deletion shouldn’t be possible without the edit_catalogue permission.
Comment 1 Lucas Gass (lukeg) 2025-05-06 22:22:19 UTC
Created attachment 182010 [details] [review]
Bug 39848: Restrict record deletion to staff who have edit_catalogue permission

To test,
1. Create a user with the following permissions: edit_any_item, edit_items, items_batchdel, items_batchmod.
2. When logged in as that user, use the batch item deletion tool to delete a few items.
3. Select “Delete records if no items remain” and “Delete selected items.”
When the job has processed, notice that X item(s) deleted. X record(s) deleted.
4. APPLY PATCH
5. With the same user repeat step 2.
6. This time there should be no option to 'Delete records if no items remain'.
7. Give the same use the Edit catalog (Modify bibliographic/holdings data) (edit_catalogue) permission.
8. Make sure you can successfully delete a record using the 'Delete records if no items remain'.
Comment 2 Roman Dolny 2025-05-07 13:59:20 UTC
Created attachment 182030 [details] [review]
Bug 39848: Restrict record deletion to staff who have edit_catalogue permission

To test,
1. Create a user with the following permissions: edit_any_item, edit_items, items_batchdel, items_batchmod.
2. When logged in as that user, use the batch item deletion tool to delete a few items.
3. Select “Delete records if no items remain” and “Delete selected items.”
When the job has processed, notice that X item(s) deleted. X record(s) deleted.
4. APPLY PATCH
5. With the same user repeat step 2.
6. This time there should be no option to 'Delete records if no items remain'.
7. Give the same use the Edit catalog (Modify bibliographic/holdings data) (edit_catalogue) permission.
8. Make sure you can successfully delete a record using the 'Delete records if no items remain'.

Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Comment 3 Martin Renvoize (ashimema) 2025-05-08 12:34:55 UTC
Created attachment 182077 [details] [review]
Bug 39848: Restrict record deletion to staff who have edit_catalogue permission

To test,
1. Create a user with the following permissions: edit_any_item, edit_items, items_batchdel, items_batchmod.
2. When logged in as that user, use the batch item deletion tool to delete a few items.
3. Select “Delete records if no items remain” and “Delete selected items.”
When the job has processed, notice that X item(s) deleted. X record(s) deleted.
4. APPLY PATCH
5. With the same user repeat step 2.
6. This time there should be no option to 'Delete records if no items remain'.
7. Give the same use the Edit catalog (Modify bibliographic/holdings data) (edit_catalogue) permission.
8. Make sure you can successfully delete a record using the 'Delete records if no items remain'.

Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 4 Katrin Fischer 2025-05-09 06:25:57 UTC
Updated description slightly - unauthorized would be a bit worse :)
Comment 5 Katrin Fischer 2025-05-09 06:26:26 UTC
Is it really enough to remove the checkbox here? Should we not also check for the flag in the script itself?
Comment 6 Katrin Fischer 2025-05-09 07:14:48 UTC
Pushed for 25.05!

Well done everyone, thank you!
Comment 7 Anneli Österman 2025-05-13 10:58:24 UTC
It seems I'm a bit late with my comment but we have used item batch deletion to delete bibliographic records on purpose. We do not want to give the edit_catalogue permission to everyone who needs to delete records because it also gives them the permission to edit records (wicht nowadays takes lots of professional competence).

With this change we would need a new permission delete_catalogue (or delete_biblio_records) to obtain our current way of deleting bibliographic records.
Comment 8 Katrin Fischer 2025-05-13 12:13:31 UTC
(In reply to Anneli Österman from comment #7)
> It seems I'm a bit late with my comment but we have used item batch deletion
> to delete bibliographic records on purpose. We do not want to give the
> edit_catalogue permission to everyone who needs to delete records because it
> also gives them the permission to edit records (wicht nowadays takes lots of
> professional competence).
> 
> With this change we would need a new permission delete_catalogue (or
> delete_biblio_records) to obtain our current way of deleting bibliographic
> records.

Hi Anneli, thanks for reaching out. 

I am sorry, that this creates a problem for your workflow and I can understand your point. I checked and was surprised to only find your new bug and no older requests for this.

I'd like to find a solution that works for all use cases, but developing a new permission at this point in cycle is a bit tricky, as we'd have to change a lot of other places too.

This has been marked as major, so for some libraries it's bad... and for you the change is bad. We are in a bit of a hard stop. What we could do is maybe halt off backporting for a bit?
Comment 9 Anneli Österman 2025-05-13 12:27:06 UTC
This won't be an immediate problem for us because we do not update to newer version in a while (we have a quite slow update schedule except for security updates). Maybe the new permission has been added before our next update. 

I found bug https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35953 and commented on that one too. We will surely help with it - at the very least by testing it.
Comment 10 Katrin Fischer 2025-05-13 12:34:28 UTC
My worry is that since this is marked "major" it will be backported to all older releases - so it could hit you sooner with a security update.
Comment 11 Anneli Österman 2025-05-13 12:42:38 UTC
I will discuss about this with colleagues tomorrow, they can make a more accurate assessment than I can. Thank you for your concern! :)
Comment 12 Anneli Österman 2025-05-16 07:11:24 UTC
Sorry about the delay. We are fine with backporting. We will take our own actions if it is needed when the time comes. :)
Comment 13 Katrin Fischer 2025-05-16 15:18:48 UTC
(In reply to Anneli Österman from comment #12)
> Sorry about the delay. We are fine with backporting. We will take our own
> actions if it is needed when the time comes. :)

+1
Comment 14 Katrin Fischer 2025-05-16 15:19:07 UTC
(In reply to Katrin Fischer from comment #5)
> Is it really enough to remove the checkbox here? Should we not also check
> for the flag in the script itself?

What about this question?
Comment 15 Lucas Gass (lukeg) 2025-05-16 15:48:28 UTC
(In reply to Katrin Fischer from comment #14)
> (In reply to Katrin Fischer from comment #5)
> > Is it really enough to remove the checkbox here? Should we not also check
> > for the flag in the script itself?
> 
> What about this question?

I 100% agree, I opened Bug 39926 and assigned it to myself. I will get a patch up ASAP!
Comment 16 Katrin Fischer 2025-05-19 16:37:54 UTC
(In reply to Lucas Gass (lukeg) from comment #15)
> (In reply to Katrin Fischer from comment #14)
> > (In reply to Katrin Fischer from comment #5)
> > > Is it really enough to remove the checkbox here? Should we not also check
> > > for the flag in the script itself?
> > 
> > What about this question?
> 
> I 100% agree, I opened Bug 39926 and assigned it to myself. I will get a
> patch up ASAP!

Thanks!