| Summary: | Accounts with just 'catalogue' permission can modify/delete holds | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Joonas Kylmälä <joonas.kylmala> |
| Component: | Staff interface | Assignee: | Marcel de Rooy <m.de.rooy> |
| Status: | Pushed to oldoldoldstable --- | QA Contact: | Testopia <testopia> |
| Severity: | critical | ||
| Priority: | P5 - low | CC: | 1joynelson, andrew, anneli.osterman, dcook, fridolin.somers, gmcharlt, jonathan.druart, lucas, m.de.rooy, nick, nugged, tomascohen, victor, wainuiwitikapark |
| Version: | Main | ||
| 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
|
|
| Circulation function: | |||
| 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 |
||
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> I think we should the buttons if the logged in patron does not have the permissions. (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? (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? On a separate patch. If it conflicts when backporting it could be skipped easily. @Marcel did you forgot to change status to signed off or do you want another signing ? (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. (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. On the API front, we use the reserveforothers permission. Which is correct? (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.. circ/waitingreserves.pl: flagsrequired => { circulate => "circulate_remaining_permissions" },
Allows you to cancel a reserve.
Welcome in the minefield of picking a permission ;) 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 ! 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? "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 %]&CancelBorrowerNumber=[% itemloo.ReservedFor.borrowernumber | html %]&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. 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. 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. 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> 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> 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 Backported to 19.11.x-security branch on the Security repo for 19.11.25 security release. Pushed to security 21.05.x for 21.05.09 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). (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. (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! |
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.