1) IndependentBranches needs to be activated - 'Prevent' 2) Make sure you have some subscriptions, set to different branches 3) Search for all subscriptions with an empty search Problem 1: If the user has the superserials permission, the search doesn't show all subscriptions. Problem 2: If the user doesn't have the superserials permission, you can still edit a subscription accessing the subsscription detail page through the serial collection page (for records with multiple subscriptions) or accessing the detail page directly. Hope I tested this correctly - would be happy if someone could confirm the bug.
Created attachment 27115 [details] [review] Proposed patch
Katrin: I confirm problem 1, and propose a solution. Note that this patch requires patch for bug 12048.
I confirmed problem 1 and 2. However, this patch only solves problem 1. The /cgi-bin/koha/serials/subscription-detail.pl?subscriptionid=## URL needs to have permissions checks added still. I can direct access the page.
Created attachment 27135 [details] [review] [SIGNED OFF] Bug 12080 Superserials permission in search result Enabling/disabling IndependentBranch syspref & superserials permission, you can see that serials search result doesn't display all subscriptions when appropriate. Fixed with this patch. Dependency: This patch must be applied after patch for bug 12048. TEST PLAN --------- 1) Apply the patch for bug 12048 (as needed -- it may be pushed) 2) Ensure you have two users: superlibrarian, non-superlibrarian with all access to the staff client except superserials. 3) Ensure you have serials belonging to a different branch than the non-superlibrarian. 3) Log into staff client as superlibrarian 4) Click 'Serials' 5) Click the 'Submit' button in the search area. -- note the number of results. 6) Log into staff client as non-superlibrarian 7) Click 'Serials' 8) Click the 'Submit' button in the search area. -- note the number should be less, note the number. 9) Give the non-superlibrarian superserials access. 10) Home -> Serials 11) Click the 'Submit' button in the search area. -- the number will still be the same at the one in step #8. 12) Apply the patch 13) Refresh the page -- the number should now match the one in step #5. Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Thanks Mark for the detailed test plan.
From C4::Serials::can_edit_subscription: or C4::Auth::haspermission( $userid, {serials => 'superserials'}), or C4::Auth::haspermission( $userid, {serials => 'edit_subscription'}), If a user has edit_subscription he cans edit a subscription (without having superserials). But or $subscription->{branchcode} eq C4::Context->userenv->{'branch'} Means: if the user is on the same branch, he cans edit it too. So a user can edit a subscription without having the edit_subscription permission... hum... Who wrote this code?? (...)
I will try to provide a global fix for this permission.
Created attachment 27218 [details] [review] Bug 12080: Fix C4::Serials::can_edit_subscription This patch adds some unit tests for the can_edit_subscription and add a new can_show_subscription subroutines.
Created attachment 27219 [details] [review] Bug 12080: Refactor can_*_subscription in C4::Serials
I tried to do something for this permission, the can_edit_subscription was broken and I think we need a can_show_subscription. I added/modified these 2 routines in 2 last patches. I will be afk for 1 week and not able to continue. So you can obsolete my patch and continue with the signoff one, or continue with my patches. I will reopen a new report with these 2 patches if you decide to fix only the SearchSubscriptions routine. Feel free to do what you want and sorry not to provide a complete patch.
(In reply to Jonathan Druart from comment #10) > I will be afk for 1 week and not able to continue. So you can > obsolete my patch and continue with the signoff one, or continue with > my patches. I will obsolete your patches. I have tested them. It works, and solves problem 2 described by Katrin. And last but not lease, you two functions add clarity to the code. But I think that another bug number, with your patch, specifically for problem 2, is more manageable for RM, especially since my patch is already dependent on bug 12048. > I will reopen a new report with these 2 patches if you decide to fix only > the SearchSubscriptions routine. See bug 12098 for attaching your patch. I'd be glad to sign-of it. Thanks.
I've reverted to Signed-off, since it was the case before Jonathan new patch, which should land now on bug 12098.
Created attachment 27279 [details] [review] [PASSED QA] Bug 12080 Superserials permission in search result Enabling/disabling IndependentBranch syspref & superserials permission, you can see that serials search result doesn't display all subscriptions when appropriate. Fixed with this patch. Dependency: This patch must be applied after patch for bug 12048. TEST PLAN --------- 1) Apply the patch for bug 12048 (as needed -- it may be pushed) 2) Ensure you have two users: superlibrarian, non-superlibrarian with all access to the staff client except superserials. 3) Ensure you have serials belonging to a different branch than the non-superlibrarian. 3) Log into staff client as superlibrarian 4) Click 'Serials' 5) Click the 'Submit' button in the search area. -- note the number of results. 6) Log into staff client as non-superlibrarian 7) Click 'Serials' 8) Click the 'Submit' button in the search area. -- note the number should be less, note the number. 9) Give the non-superlibrarian superserials access. 10) Home -> Serials 11) Click the 'Submit' button in the search area. -- the number will still be the same at the one in step #8. 12) Apply the patch 13) Refresh the page -- the number should now match the one in step #5. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27284 [details] [review] Bug 12080 [QA Followup] - Bookseller.t fails
Created attachment 27286 [details] [review] Bug 12080 [QA Followup] - Bookseller.t fails
Created attachment 27287 [details] [review] Bug 12080 [QA Followup] - Bookseller.t fails Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Fixes the tests as promised.
I am aware the new follow up changes Serials.pm - I am going to test this altogether with bug 12048 on bug 12098.
Pushed to master. Thanks, Frédéric!
Pushed to 3.14.x, will be in 3.14.10