Bug 12080 - Superserials permission appears to be broken
Summary: Superserials permission appears to be broken
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Frédéric Demians
QA Contact: Testopia
URL:
Keywords:
Depends on: 12048
Blocks: 12098
  Show dependency treegraph
 
Reported: 2014-04-14 20:58 UTC by Katrin Fischer
Modified: 2015-06-04 23:30 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Proposed patch (1.72 KB, patch)
2014-04-15 08:15 UTC, Frédéric Demians
Details | Diff | Splinter Review
[SIGNED OFF] Bug 12080 Superserials permission in search result (2.66 KB, patch)
2014-04-15 15:25 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 12080: Fix C4::Serials::can_edit_subscription (13.98 KB, patch)
2014-04-16 15:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12080: Refactor can_*_subscription in C4::Serials (3.39 KB, patch)
2014-04-16 15:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 12080 Superserials permission in search result (2.71 KB, patch)
2014-04-18 11:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12080 [QA Followup] - Bookseller.t fails (1.70 KB, patch)
2014-04-18 12:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12080 [QA Followup] - Bookseller.t fails (1.70 KB, patch)
2014-04-18 12:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12080 [QA Followup] - Bookseller.t fails (1.78 KB, patch)
2014-04-18 12:56 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2014-04-14 20:58:35 UTC
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.
Comment 1 Frédéric Demians 2014-04-15 08:15:10 UTC Comment hidden (obsolete)
Comment 2 Frédéric Demians 2014-04-15 08:16:18 UTC
Katrin: I confirm problem 1, and propose a solution. Note that this patch requires patch for bug 12048.
Comment 3 Mark Tompsett 2014-04-15 15:22:38 UTC
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.
Comment 4 Mark Tompsett 2014-04-15 15:25:07 UTC Comment hidden (obsolete)
Comment 5 Frédéric Demians 2014-04-15 16:57:46 UTC
Thanks Mark for the detailed test plan.
Comment 6 Jonathan Druart 2014-04-16 12:02:04 UTC
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??
(...)
Comment 7 Jonathan Druart 2014-04-16 13:21:06 UTC
I will try to provide a global fix for this permission.
Comment 8 Jonathan Druart 2014-04-16 15:00:59 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2014-04-16 15:01:05 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2014-04-16 15:05:01 UTC
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.
Comment 11 Frédéric Demians 2014-04-16 19:17:03 UTC
(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.
Comment 12 Frédéric Demians 2014-04-16 19:19:07 UTC
I've reverted to Signed-off, since it was the case before Jonathan new patch, which should land now on bug 12098.
Comment 13 Kyle M Hall 2014-04-18 11:33:02 UTC
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>
Comment 14 Kyle M Hall 2014-04-18 12:28:12 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2014-04-18 12:36:53 UTC Comment hidden (obsolete)
Comment 16 Katrin Fischer 2014-04-18 12:56:13 UTC
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.
Comment 17 Katrin Fischer 2014-04-18 12:58:56 UTC
I am aware the new follow up changes Serials.pm - I am going to test this altogether with bug 12048 on bug 12098.
Comment 18 Galen Charlton 2014-04-18 20:50:54 UTC
Pushed to master.  Thanks, Frédéric!
Comment 19 Fridolin Somers 2014-08-07 08:24:53 UTC
Pushed to 3.14.x, will be in 3.14.10