Bug 7002 - Inaccurate checks for borrower flags == 1
Summary: Inaccurate checks for borrower flags == 1
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Galen Charlton
QA Contact: Bugs List
URL:
Keywords:
Depends on: 10277
Blocks: 6390
  Show dependency treegraph
 
Reported: 2011-10-11 14:47 UTC by Owen Leonard
Modified: 2015-06-04 23:30 UTC (History)
4 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
Bug 7002: fix some invalid superlibrarian permission checks (3.26 KB, patch)
2013-12-30 16:12 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 7002: fix some invalid superlibrarian permission checks (3.30 KB, patch)
2014-01-02 04:28 UTC, Chris Cormack
Details | Diff | Splinter Review
[PASSED QA] Bug 7002: fix some invalid superlibrarian permission checks (3.44 KB, patch)
2014-01-05 21:02 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 Owen Leonard 2011-10-11 14:47:00 UTC
There are a few instances in the code which I can find which use this to check whether someone has superlibrarian privileges in the context of IndependandBranches:

unless (($userenv->{'flags'} == 1) or ($userenv->{'branch'} eq $item->{'homebranch'})) {

This check fails if the logged in user has any other privilege checked in addition to superlibrarian. In other words, the user must *only* have superlibrarian checked in order to qualify to perform these operations.

catalogue\moredetail.pl line 136
cataloguing\additem.pl line 548
tools\batchMod.pl line 460
Comment 1 Ian Walls 2011-10-11 15:33:55 UTC
To check for superlibrarian (or any top-level permission), you need to use bitwise AND.

($user->{'flags'} & 1) should return true of the user has superlibrarian checked, and false otherwise.

C4/Serials.pm does similar erroneous checks for ($user->{'flags'} != 1), which will also need modification.
Comment 2 Katrin Fischer 2013-12-29 15:42:14 UTC
Hm, I did a quick search and found some occurences of the problematic syntax in the current code:

catalogue/moredetail.pl	
178         unless (($userenv->{'flags'} == 1) or ($userenv->{'branch'} eq $item->{'homebranch'})) {

cataloguing/additem.pl	
695             unless (($userenv->{'flags'} == 1) or (($userenv->{'branch'} eq $subfieldvalue))){

tools/batchMod.pl	
498                                                 unless (($userenv->{'flags'} == 1) or (($userenv->{'branch'} eq $itembranchcode))){
Comment 3 Galen Charlton 2013-12-30 16:12:50 UTC Comment hidden (obsolete)
Comment 4 Chris Cormack 2014-01-02 04:28:09 UTC Comment hidden (obsolete)
Comment 5 Katrin Fischer 2014-01-05 21:02:03 UTC
Created attachment 23907 [details] [review]
[PASSED QA] Bug 7002: fix some invalid superlibrarian permission checks

This patch fixes a problem where if a staff user has superlibrarian
permissions, but also has module-specific permissions, they are
prevent from editing item records that they should be allowed to.

To test:

[1] Turn on IndependentBranches.
[2] Register a superlibrarian staff user at branch A.
[3] Give that new account at least one other module-level
    permission.  This cannot be done through the user interface,
    however, but can be done via SQL:

    UPDATE borrowers SET flags = 3 WHERE userid = 'XXX';

[4] Log in as that new superlibrarian.
[5] Bring up the item details (catalogue/moredetail.pl) page for
    an item at branch B.  Note that there is no 'Edit Item' link.
[6] Similarly, try editing that item (cataloging/additem.pl).  Note
    that the edit form forbids you from touching the item.
[7] Finally, try editing that item using the Tools | Batch item
    modification utility.  Note that it doesn't allow you to do so.
[8] Apply the patch.
[9] Repeat steps 5 through 7.  This time, the item actions should
    be allowed.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Works as described, passes QA script and test suite.
Comment 6 Galen Charlton 2014-01-06 05:51:16 UTC
Pushed to master.
Comment 7 Katrin Fischer 2014-01-06 09:41:18 UTC
Thanks Galen :)
Comment 8 Fridolin Somers 2014-02-18 07:53:47 UTC
Will be for 3.16 because depends on enhancement Bug 10277
Comment 9 Fridolin Somers 2014-08-07 07:49:49 UTC
Pushed to 3.14.x, will be in 3.14.10

I decided in the end to integrate Bug 10277 so also this one.