|
Description
Kyle M Hall (khall)
2022-03-01 16:49:21 UTC
Created attachment 131203 [details] [review] Bug 30207: Librarians with only "place_holds" permission can no longer update hold pickup locations Bug 29540 increased the flags required to for modreserve.pl, but in doing so created a situation where a library can select a different pickup location for a hold, but upon clicking the "Update hold(s)" button, will be logged out due to having insufficient permissions for modreserve.pl. Test Plan: 1) Create a librarian with "place_holds" permissions, but not "modify_holds_priority". 2) Place a hold for a patron 3) Attempt to change the pickup location from request.pl 4) Note you get logged out with a permissions error 5) Apply this patch 6) Restart all the things! 7) Attempt to change the pickup location from request.pl 8) This time it should succeed! Created attachment 131205 [details] [review] Bug 30207: Librarians with only "place_holds" permission can no longer update hold pickup locations Bug 29540 increased the flags required to for modreserve.pl, but in doing so created a situation where a library can select a different pickup location for a hold, but upon clicking the "Update hold(s)" button, will be logged out due to having insufficient permissions for modreserve.pl. Test Plan: 1) Create a librarian with "place_holds" permissions, but not "modify_holds_priority". 2) Place a hold for a patron 3) Attempt to change the pickup location from request.pl 4) Note you get logged out with a permissions error 5) Apply this patch 6) Restart all the things! 7) Attempt to change the pickup location from request.pl 8) This time it should succeed! Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org> Works as described. When I try this, a user with only "place_holds" permission can add, suspend, or delete a hold. The user cannot change the hold date, the expiration date, or the pickup location. Is this to be expected? I would have expected that a user with "place_holds" but NOT "modify_holds_priority" would be able to make any change EXCEPT change priority. Never mind, I wasn't on the branch I thought I was! Sorry for the noise. Hi Kyle, quick question:
could it make sense to use:
flagsrequired => { reserveforothers => '*' },
As I understand it, that means "module level or any sub permission". So that could be more fail save if we added more sub perms.
I don't think this is 100% accurate. The script is also about cancellation, suspend, modifying pickup library, etc. In my understanding if you have modify_holds_priority you should only be allowed to modify the priority. (In reply to Jonathan Druart from comment #7) > I don't think this is 100% accurate. The script is also about cancellation, > suspend, modifying pickup library, etc. > > In my understanding if you have modify_holds_priority you should only be > allowed to modify the priority. This patch should not be touching any other aspect of holds. If it is, it wasn't caught through the testing plan. If it is allowing more that changing priorities, then it should fail. Does this fix the problem that staff cannot cancel holds from the patron detail page / checkout page if they have only place_hold permission? The test plan only mentions request.pl page. +1 to Anneli's comment. This appears to have also broken hold cancellation; where staff can no longer cancel holds from the patron details unless they have modify_holds_priority permission. We want that permission to be very rare in our system, but we want most staff to have the ability to cancel holds. Created attachment 134808 [details] [review] Bug 30207: Allow any reserveforothers subpermission to access the page I tested the new patch and now you can delete hold from patron details or check out pages with the permission place_holds. So that is ok now. However, I noticed that the suspend/resume functions doesn't work correctly on patron details/check out pages. When I tried to suspend or resume one hold, Koha showed me a pop up "Your request could not be processed. Check the logs" and OK button. When I tried to suspend or resume all holds, Koha gave me log in page with the message "Error: You do not have permission to access this page." Suspend/resume works fine at the request.pl page (In reply to Anneli Österman from comment #12) > I tested the new patch and now you can delete hold from patron details or > check out pages with the permission place_holds. So that is ok now. > > However, I noticed that the suspend/resume functions doesn't work correctly > on patron details/check out pages. > > When I tried to suspend or resume one hold, Koha showed me a pop up "Your > request could not be processed. Check the logs" and OK button. > > When I tried to suspend or resume all holds, Koha gave me log in page with > the message "Error: You do not have permission to access this page." > > Suspend/resume works fine at the request.pl page Good catch! What you are seeing is not related to request.pl. Instead it has something to do with modrequest_suspendall.pl and is outside the scope of this bug report. I'll file a separate bug for that issue and link it to this one. (In reply to Anneli Österman from comment #12) > I tested the new patch and now you can delete hold from patron details or > check out pages with the permission place_holds. So that is ok now. > > However, I noticed that the suspend/resume functions doesn't work correctly > on patron details/check out pages. > > When I tried to suspend or resume one hold, Koha showed me a pop up "Your > request could not be processed. Check the logs" and OK button. > > When I tried to suspend or resume all holds, Koha gave me log in page with > the message "Error: You do not have permission to access this page." > > Suspend/resume works fine at the request.pl page Good catch! What you are seeing is not related to request.pl. Instead it has something to do with modrequest_suspendall.pl and is outside the scope of this bug report. I'll file a separate bug for that issue and link it to this one. Created attachment 134977 [details] [review] Bug 30207: Librarians with only "place_holds" permission can no longer update hold pickup locations Bug 29540 increased the flags required to for modreserve.pl, but in doing so created a situation where a library can select a different pickup location for a hold, but upon clicking the "Update hold(s)" button, will be logged out due to having insufficient permissions for modreserve.pl. Test Plan: 1) Create a librarian with "place_holds" permissions, but not "modify_holds_priority". 2) Place a hold for a patron 3) Attempt to change the pickup location from request.pl 4) Note you get logged out with a permissions error 5) Apply this patch 6) Restart all the things! 7) Attempt to change the pickup location from request.pl 8) This time it should succeed! Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 134978 [details] [review] Bug 30207: Allow any reserveforothers subpermission to access the page Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Pushed to master for 22.05, thanks to everybody involved [U+1F984] Hi, Possible to backport this one into 21.11? Thanks! Liz Hi, Applied to 21.11.x for 21.11.11 :) thx! Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed. No changes to the Manual needed. |