Summary: | Superserials permission appears to be broken | ||
---|---|---|---|
Product: | Koha | Reporter: | Katrin Fischer <katrin.fischer> |
Component: | Serials | Assignee: | Frédéric Demians <f.demians> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | major | ||
Priority: | P5 - low | CC: | colin.campbell, f.demians, fridolin.somers, gmcharlt, jonathan.druart, mtompset |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | Small patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Bug Depends on: | 12048 | ||
Bug Blocks: | 12098 | ||
Attachments: |
Proposed patch
[SIGNED OFF] Bug 12080 Superserials permission in search result Bug 12080: Fix C4::Serials::can_edit_subscription Bug 12080: Refactor can_*_subscription in C4::Serials [PASSED QA] Bug 12080 Superserials permission in search result Bug 12080 [QA Followup] - Bookseller.t fails Bug 12080 [QA Followup] - Bookseller.t fails Bug 12080 [QA Followup] - Bookseller.t fails |
Description
Katrin Fischer
2014-04-14 20:58:35 UTC
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 |