Bug 29540

Summary: Accounts with just 'catalogue' permission can modify/delete holds
Product: Koha Reporter: Joonas Kylmälä <joonas.kylmala>
Component: Staff interfaceAssignee: Marcel de Rooy <m.de.rooy>
Status: Pushed to oldoldoldstable --- QA Contact: Testopia <testopia>
Severity: critical    
Priority: P5 - low CC: andrewfh, anneli.osterman, dcook, fridolin.somers, gmcharlt, jonathan.druart+koha, joy, katrin.fischer, lucas, m.de.rooy, martin.renvoize, nick, nugged, tomascohen, victor, wainuiwitikapark
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30213
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
Bug Depends on:    
Bug Blocks: 29817, 30207    
Attachments: Bug 29540: Raise flagsrequired in modrequest
Bug 29540: Raise flagsrequired in modrequest
Bug 29540: Raise flagsrequired in modrequest
Bug 29540: Raise flagsrequired in modrequest

Description Joonas Kylmälä 2021-11-20 19:10:36 UTC
The scripts:

reserve/modrequest.pl
reserve/modrequest_suspendall.pl

only require the 'catalogue' permission, i.e. minimum permission required for staff interface login. This leads to now everybody who has a staff account but no permission to modify or delete holds being able to do that. The scripts should require one of the following permissions depending on the action:

- reserveforothers
  - place_hold
  - modify_holds_priority

Example reproducer:

1. login to intranet with an account that has just catalogue permission
2. Know some borrowernumber before hand who has holds
3. Go to url

> localhost:8081/cgi-bin/koha/reserve/modrequest_suspendall.pl?borrowernumber=XXXXX&suspend=1

4. Notice the patron has now all their holds suspended.
Comment 1 Marcel de Rooy 2021-11-22 08:14:02 UTC
Created attachment 127915 [details] [review]
Bug 29540: Raise flagsrequired in modrequest

Same level as circulation.pl

Test plan:
Try modrequest with a user having only 'catalogue' perms and the following URLs:
[1] /cgi-bin/koha/reserve/modrequest.pl?reserve_id=XX&CancelBorrowerNumber=XX&CancelItemnumber=XX&biblionumber=XX
    Fill the XXs with correct identifiers for some item level hold.
[2] /cgi-bin/koha/reserve/modrequest_suspendall.pl?suspend=1&suspend_until=2021-12-01&borrowernumber=XX
    Fill the XX with borrowernumber for borrower that has pending holds.
You should see: Error: You do not have permission to access this page.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Jonathan Druart 2021-11-22 10:20:56 UTC
I think we should the buttons if the logged in patron does not have the permissions.
Comment 3 Marcel de Rooy 2021-11-22 12:33:55 UTC
(In reply to Jonathan Druart from comment #2)
> I think we should the buttons if the logged in patron does not have the
> permissions.

Should we better do that interface stuff/templating on a regular report?
Comment 4 Jonathan Druart 2021-11-24 09:14:25 UTC
(In reply to Marcel de Rooy from comment #3)
> (In reply to Jonathan Druart from comment #2)
> > I think we should the buttons if the logged in patron does not have the
> > permissions.
> 
> Should we better do that interface stuff/templating on a regular report?

Why not here? That seems quite trivial, isn't it?
Comment 5 Jonathan Druart 2021-11-24 09:15:13 UTC
On a separate patch. If it conflicts when backporting it could be skipped easily.
Comment 6 Fridolin Somers 2021-12-06 22:56:52 UTC
@Marcel did you forgot to change status to signed off or do you want another signing ?
Comment 7 Marcel de Rooy 2021-12-07 07:34:38 UTC
(In reply to Fridolin Somers from comment #6)
> @Marcel did you forgot to change status to signed off or do you want another
> signing ?

I wrote this patch. It needs a signoff.
Comment 8 Fridolin Somers 2021-12-08 06:27:46 UTC
(In reply to Marcel de Rooy from comment #7)
> (In reply to Fridolin Somers from comment #6)
> > @Marcel did you forgot to change status to signed off or do you want another
> > signing ?
> 
> I wrote this patch. It needs a signoff.

Ah ok sure.
Comment 9 Tomás Cohen Arazi 2021-12-13 11:57:29 UTC
On the API front, we use the reserveforothers permission. Which is correct?
Comment 10 Marcel de Rooy 2021-12-13 12:45:25 UTC
(In reply to Tomás Cohen Arazi from comment #9)
> On the API front, we use the reserveforothers permission. Which is correct?

Good observation! Koha is a bit inconsistent/hybrid here. Two examples:

reserve/request.pl
flagsrequired   => { reserveforothers => 'place_holds' },

koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt
[% IF ( CAN_user_circulate_circulate_remaining_permissions ) %]
                                <li>
                                    [% IF ( holds_count ) %]
                                        <a href="#reserves" id="holds-tab">[% holds_count | html %] Hold(s)</a>
ETC.

My first impression is to move to reserveforothers now..
Comment 11 Marcel de Rooy 2021-12-13 12:47:04 UTC
circ/waitingreserves.pl:        flagsrequired   => { circulate => "circulate_remaining_permissions" },

Allows you to cancel a reserve.
Comment 12 Marcel de Rooy 2021-12-13 12:47:30 UTC
Welcome in the minefield of picking a permission ;)
Comment 13 Marcel de Rooy 2021-12-13 13:02:10 UTC
Hmm. On second thought I may be inclined to interpret reserveforothers as a more restrictive permission: place a hold or modify its priority.

What we do in modrequest is two things: cancel a hold, or alter multiple holds (possibly suspend or cancel them). These things are imo asking for more permission than reserveforothers. And thats where circulate_remaining pops up: these remaining 'collection boxes' allow you to do *almost anything that you forgot to define explicitly* :)

QA team, please help us out of the discussion !
Comment 14 Katrin Fischer 2021-12-13 13:18:55 UTC
I have a bit of a different understanding here:

(reserveforothers) is the top level permission. So for me this translates to: all things about about managing holds.
Then we have 2 sub permissions (modify_holds_priority) and (place_holds) that are for subsets of "managing holds".

I don't think circulation permissions should come into play here?
Comment 15 Marcel de Rooy 2021-12-22 14:31:25 UTC
"Could you also disable a few buttons"? Well, it sounds simple but it seems to be much more work.

koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt:                            <form action="/cgi-bin/koha/reserve/modrequest.pl" method="post">
koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt:                                    <form action="/cgi-bin/koha/reserve/modrequest_suspendall.pl" method="post">
koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt:                                    <form action="/cgi-bin/koha/reserve/modrequest_suspendall.pl" method="post">

koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt:                                    <form action="/cgi-bin/koha/reserve/modrequest.pl" method="post">
koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt:                                        <form action="/cgi-bin/koha/reserve/modrequest_suspendall.pl" method="post">
koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt:                                        <form action="/cgi-bin/koha/reserve/modrequest_suspendall.pl" method="post">

koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt:                                                                <a class="info cancel-hold" href="modrequest.pl?CancelBiblioNumber=[% itemloo.biblionumber | html %]&amp;CancelBorrowerNumber=[% itemloo.ReservedFor.borrowernumber | html %]&amp;CancelItemnumber=[% itemloo.itemnumber | html %]">Cancel hold</a>
koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt:                    <form id="existing_holds" name="T[% time | html %]" action="modrequest.pl" method="post" style="display:block">

koha-tmpl/opac-tmpl/bootstrap/en/includes/holds-table.inc:                                        <form class="form-inline" action="/cgi-bin/koha/opac-modrequest-suspend.pl" method="post">
koha-tmpl/opac-tmpl/bootstrap/en/includes/holds-table.inc:                                                        <form action="/cgi-bin/koha/opac-modrequest-suspend.pl" method="post">
koha-tmpl/opac-tmpl/bootstrap/en/includes/holds-table.inc:                                            <form class="form-inline" action="/cgi-bin/koha/opac-modrequest-suspend.pl" method="post">
koha-tmpl/opac-tmpl/bootstrap/en/includes/holds-table.inc:                                    <form id="delete_hold_[% HOLD.reserve_id | html %]" action="/cgi-bin/koha/opac-modrequest.pl" method="post">
koha-tmpl/opac-tmpl/bootstrap/en/includes/holds-table.inc:                    <form class="form-inline" id="suspend_all_holds" action="/cgi-bin/koha/opac-modrequest-suspend.pl" method="post">
koha-tmpl/opac-tmpl/bootstrap/en/includes/holds-table.inc:                    <form id="resume_all_holds" action="/cgi-bin/koha/opac-modrequest-suspend.pl" method="post">

So I prefer to stick to this simple adjustment here.
Comment 16 Marcel de Rooy 2021-12-22 14:32:10 UTC
Created attachment 128861 [details] [review]
Bug 29540: Raise flagsrequired in modrequest

Test plan:
Try modrequest with a user having only 'catalogue' perms and the following URLs:
[1] /cgi-bin/koha/reserve/modrequest.pl?reserve_id=XX&CancelBorrowerNumber=XX&CancelItemnumber=XX&biblionumber=XX
    Fill the XXs with correct identifiers for some item level hold.
[2] /cgi-bin/koha/reserve/modrequest_suspendall.pl?suspend=1&suspend_until=2021-12-01&borrowernumber=XX
    Fill the XX with borrowernumber for borrower that has pending holds.
You should see: Error: You do not have permission to access this page.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[AMENDED] More consensus for using reserveforothers than circulate_remaining.
Comment 17 Katrin Fischer 2022-01-02 16:16:23 UTC
Testing this I notice that the holds tab within the patron account is not even visible without reserveforothers, so I believe we are on the right track.
Comment 18 Katrin Fischer 2022-01-02 16:25:53 UTC
Created attachment 128952 [details] [review]
Bug 29540: Raise flagsrequired in modrequest

Test plan:
Try modrequest with a user having only 'catalogue' perms and the following URLs:
[1] /cgi-bin/koha/reserve/modrequest.pl?reserve_id=XX&CancelBorrowerNumber=XX&CancelItemnumber=XX&biblionumber=XX
    Fill the XXs with correct identifiers for some item level hold.
[2] /cgi-bin/koha/reserve/modrequest_suspendall.pl?suspend=1&suspend_until=2021-12-01&borrowernumber=XX
    Fill the XX with borrowernumber for borrower that has pending holds.
You should see: Error: You do not have permission to access this page.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[AMENDED] More consensus for using reserveforothers than circulate_remaining.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 19 Jonathan Druart 2022-01-07 10:44:55 UTC
Created attachment 129135 [details] [review]
Bug 29540: Raise flagsrequired in modrequest

Test plan:
Try modrequest with a user having only 'catalogue' perms and the following URLs:
[1] /cgi-bin/koha/reserve/modrequest.pl?reserve_id=XX&CancelBorrowerNumber=XX&CancelItemnumber=XX&biblionumber=XX
    Fill the XXs with correct identifiers for some item level hold.
[2] /cgi-bin/koha/reserve/modrequest_suspendall.pl?suspend=1&suspend_until=2021-12-01&borrowernumber=XX
    Fill the XX with borrowernumber for borrower that has pending holds.
You should see: Error: You do not have permission to access this page.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[AMENDED] More consensus for using reserveforothers than circulate_remaining.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 20 Victor Grousset/tuxayo 2022-01-24 00:21:58 UTC
Backported: Pushed to 20.11.x-security branch for 20.11.14

testing notes:
even if one sees "You do not have permission to access this page", on must check if the hold request is still there/unsuspended.

for [1] no need to have the item waiting so no need for itemnumber in the URL
http://localhost:8081/cgi-bin/koha/reserve/modrequest.pl?reserve_id=XXXX&CancelBorrowerNumber=XXXX&biblionumber=XXXX
Comment 21 wainuiwitikapark 2022-01-25 03:10:36 UTC
Backported to 19.11.x-security branch on the Security repo for 19.11.25 security release.
Comment 22 Andrew Fuerste-Henry 2022-01-26 18:19:02 UTC
Pushed to security 21.05.x for 21.05.09
Comment 23 Anneli Österman 2022-03-22 12:50:38 UTC
We have a problem which is probably related to this bug. Our staff is not allowed to change the priority of holds and they only have place_hold permission. Now they cannot delete/cancel holds when they try it from the patron's details/checkout page. They are redirected to the modrequest.pl page which requires reserveforothers permission which they do not have. We do not want to give this permission to everyone because then they will get the modify_holds_priority permission also.

The staff members can delete/cancel holds from the request.pl page but it would be good if they could do it on both places as previously (in older Koha versions).
Comment 24 Jonathan Druart 2022-03-22 13:15:33 UTC
(In reply to Anneli Österman from comment #23)
> We have a problem which is probably related to this bug. Our staff is not
> allowed to change the priority of holds and they only have place_hold
> permission. Now they cannot delete/cancel holds when they try it from the
> patron's details/checkout page. They are redirected to the modrequest.pl
> page which requires reserveforothers permission which they do not have. We
> do not want to give this permission to everyone because then they will get
> the modify_holds_priority permission also.
> 
> The staff members can delete/cancel holds from the request.pl page but it
> would be good if they could do it on both places as previously (in older
> Koha versions).

This has been reported on bug 30207.
Comment 25 Anneli Österman 2022-03-22 13:19:48 UTC
(In reply to Jonathan Druart from comment #24)
> (In reply to Anneli Österman from comment #23)
> > We have a problem which is probably related to this bug. Our staff is not
> > allowed to change the priority of holds and they only have place_hold
> > permission. Now they cannot delete/cancel holds when they try it from the
> > patron's details/checkout page. They are redirected to the modrequest.pl
> > page which requires reserveforothers permission which they do not have. We
> > do not want to give this permission to everyone because then they will get
> > the modify_holds_priority permission also.
> > 
> > The staff members can delete/cancel holds from the request.pl page but it
> > would be good if they could do it on both places as previously (in older
> > Koha versions).
> 
> This has been reported on bug 30207.

Thanks, I'll check that!