Bug 29903 - Message deletion possible from different branch
Summary: Message deletion possible from different branch
Status: Pushed to oldoldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
: 28724 (view as bug list)
Depends on:
Blocks:
 
Reported: 2022-01-19 10:21 UTC by Jonathan Druart
Modified: 2022-02-21 22:06 UTC (History)
14 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.02,21.05.09,20.11.14,19.11.25


Attachments
Bug 29903: Prevent messages to be deleted from unauthorised users (1.33 KB, patch)
2022-01-19 10:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29903: Prevent messages to be deleted from unauthorised users (1.35 KB, patch)
2022-01-19 13:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29903: Prevent messages to be deleted from unauthorised users (1.29 KB, patch)
2022-01-19 14:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29903: Prevent messages to be deleted from unauthorised users (1.34 KB, patch)
2022-01-19 14:49 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2022-01-19 10:21:09 UTC
The "Delete" link is hidden but the controller does not do the necessary checks.

/cgi-bin/koha/circ/del_message.pl?message_id=2&borrowernumber=5&from=moremember
Comment 1 Jonathan Druart 2022-01-19 10:23:58 UTC
Created attachment 129594 [details] [review]
Bug 29903: Prevent messages to be deleted from unauthorised users

The "Delete" link is hidden but the controller does not do the necessary checks.

/cgi-bin/koha/circ/del_message.pl?message_id=1&borrowernumber=5&from=moremember

Test plan:
Create a message, see the "Delete" link, don't click it but copy it
Change logged in library and use the link
If AllowAllMessageDeletion is off you should be redirected to 403
Comment 2 Nick Clemens 2022-01-19 13:21:06 UTC
I think you want &&, not || - with your patch I can't delete any messages

I also think we need another bug, the delete link only shows if the branchcode of the patron matches the branchcode of the message. I.e. you only get a link if a Centerville patron has a note from Centerville - it should probably check the logged in branchcode against the message branchcode:

koha-tmpl/intranet-tmpl/prog/en/includes/patron_messages.inc
220                     [% IF patron_message.branchcode == patron.branchcode OR Koha.Preference('AllowAllMessageDeleti    on') %]
Comment 3 Jonathan Druart 2022-01-19 13:50:38 UTC
(In reply to Nick Clemens from comment #2)
> I think you want &&, not || - with your patch I can't delete any messages

Oops yes, last minute change switching from "unless" to "if"...

> I also think we need another bug, the delete link only shows if the
> branchcode of the patron matches the branchcode of the message. I.e. you
> only get a link if a Centerville patron has a note from Centerville - it
> should probably check the logged in branchcode against the message
> branchcode:
> 
> koha-tmpl/intranet-tmpl/prog/en/includes/patron_messages.inc
> 220                     [% IF patron_message.branchcode == patron.branchcode
> OR Koha.Preference('AllowAllMessageDeleti    on') %]

It's bug 29889.
Comment 4 Jonathan Druart 2022-01-19 13:50:56 UTC
Created attachment 129611 [details] [review]
Bug 29903: Prevent messages to be deleted from unauthorised users

The "Delete" link is hidden but the controller does not do the necessary checks.

/cgi-bin/koha/circ/del_message.pl?message_id=1&borrowernumber=5&from=moremember

Test plan:
Create a message, see the "Delete" link, don't click it but copy it
Change logged in library and use the link
If AllowAllMessageDeletion is off you should be redirected to 403
Comment 5 Nick Clemens 2022-01-19 14:14:26 UTC
This patch checks the homebranch of the logged in librarian - bug 29889 checks the currently logged in library

This means a staff from centerville can only delete centerville messages, no matter where they are logged in. The link shows though if you are logged in at the branch that left the message
Comment 6 Jonathan Druart 2022-01-19 14:36:58 UTC
Created attachment 129613 [details] [review]
Bug 29903: Prevent messages to be deleted from unauthorised users

The "Delete" link is hidden but the controller does not do the necessary checks.

/cgi-bin/koha/circ/del_message.pl?message_id=1&borrowernumber=5&from=moremember

Test plan:
Create a message, see the "Delete" link, don't click it but copy it
Change logged in library and use the link
If AllowAllMessageDeletion is off you should be redirected to 403
Comment 7 Nick Clemens 2022-01-19 14:49:41 UTC
Created attachment 129614 [details] [review]
Bug 29903: Prevent messages to be deleted from unauthorised users

The "Delete" link is hidden but the controller does not do the necessary checks.

/cgi-bin/koha/circ/del_message.pl?message_id=1&borrowernumber=5&from=moremember

Test plan:
Create a message, see the "Delete" link, don't click it but copy it
Change logged in library and use the link
If AllowAllMessageDeletion is off you should be redirected to 403

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 8 Nick Clemens 2022-01-19 14:50:19 UTC
Straightforward, passing QA
Comment 9 Fridolin Somers 2022-01-19 21:50:57 UTC
I've pushed Bug 29889 so that behavior is consistent when this one is pushed.
Comment 10 Victor Grousset/tuxayo 2022-01-27 23:08:56 UTC
Backported: Pushed to 20.11.x branch for 20.11.14
Comment 11 Victor Grousset/tuxayo 2022-01-28 11:29:43 UTC
*20.11.x-security branch
Comment 12 wainuiwitikapark 2022-01-30 23:59:12 UTC
Backported to 19.11.x-security branch on the Security repo for 19.11.25 security release.
Comment 13 George Williams (NEKLS) 2022-02-21 22:06:40 UTC
*** Bug 28724 has been marked as a duplicate of this bug. ***