Bug 29903

Summary: Message deletion possible from different branch
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Staff interfaceAssignee: Jonathan Druart <jonathan.druart>
Status: Pushed to oldoldoldstable --- QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: 1joynelson, andrewfh, dcook, fridolin.somers, george, gmcharlt, jonathan.druart, katrin.fischer, lucas, m.de.rooy, martin.renvoize, nick, victor, wainuiwitikapark
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29889
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28724
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
Bug 29903: Prevent messages to be deleted from unauthorised users
Bug 29903: Prevent messages to be deleted from unauthorised users
Bug 29903: Prevent messages to be deleted from unauthorised users

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. ***