Bug 30207

Summary: Librarians with only "place_holds" permission can no longer update hold pickup locations
Product: Koha Reporter: Kyle M Hall <kyle>
Component: Hold requestsAssignee: Kyle M Hall <kyle>
Status: RESOLVED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: acampbell, anneli.osterman, arthur.suzuki, aude.charillon, cbrannon, gmcharlt, jonathan.druart, m.de.rooy, victor, wizzyrea
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30732
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30780
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00, 21.11.11
Bug Depends on: 29540    
Bug Blocks: 30213    
Attachments: Bug 30207: Librarians with only "place_holds" permission can no longer update hold pickup locations
Bug 30207: Librarians with only "place_holds" permission can no longer update hold pickup locations
Bug 30207: Allow any reserveforothers subpermission to access the page
Bug 30207: Librarians with only "place_holds" permission can no longer update hold pickup locations
Bug 30207: Allow any reserveforothers subpermission to access the page

Description Kyle M Hall 2022-03-01 16:49:21 UTC
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.
Comment 1 Kyle M Hall 2022-03-01 16:53:44 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!
Comment 2 ByWater Sandboxes 2022-03-01 17:52:08 UTC
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>
Comment 3 Christopher Brannon 2022-03-01 17:52:36 UTC
Works as described.
Comment 4 Owen Leonard 2022-03-03 14:04:45 UTC
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.
Comment 5 Owen Leonard 2022-03-03 14:06:16 UTC
Never mind, I wasn't on the branch I thought I was! Sorry for the noise.
Comment 6 Katrin Fischer 2022-03-06 13:01:01 UTC
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.
Comment 7 Jonathan Druart 2022-03-21 13:38:06 UTC
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.
Comment 8 Christopher Brannon 2022-03-21 14:54:28 UTC
(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.
Comment 9 Anneli Österman 2022-03-22 13:30:50 UTC
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.
Comment 10 Michael Adamyk 2022-05-09 20:40:12 UTC
+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.
Comment 11 Kyle M Hall 2022-05-10 10:56:45 UTC
Created attachment 134808 [details] [review]
Bug 30207: Allow any reserveforothers subpermission to access the page
Comment 12 Anneli Österman 2022-05-11 12:38:50 UTC
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
Comment 13 Kyle M Hall 2022-05-11 13:45:24 UTC
(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.
Comment 14 Kyle M Hall 2022-05-11 13:57:56 UTC
(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.
Comment 15 Katrin Fischer 2022-05-13 15:15:37 UTC
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>
Comment 16 Katrin Fischer 2022-05-13 15:15:42 UTC
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>
Comment 17 Fridolin Somers 2022-05-14 01:42:46 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 18 Liz Rea 2022-07-18 16:11:03 UTC
Hi, 

Possible to backport this one into 21.11?

Thanks!

Liz
Comment 19 Arthur Suzuki 2022-07-21 11:27:18 UTC
Hi,
Applied to 21.11.x for 21.11.11 :)
thx!
Comment 20 Victor Grousset/tuxayo 2022-07-23 19:53:39 UTC
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.
Comment 21 Aude Charillon 2023-05-12 14:19:15 UTC
No changes to the Manual needed.