Bug 6885

Summary: Superlibrarian users can't delete items from another library when IndependantBranches
Product: Koha Reporter: Frédéric Demians <f.demians>
Component: CatalogingAssignee: Frédéric Demians <f.demians>
Status: CLOSED FIXED QA Contact: Bugs List <koha-bugs>
Severity: normal    
Priority: P3 CC: chris, julian.maurice, katrin.fischer, koha.sekjal, michael.hafen, nengard, paul.poulain
Version: 3.6   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Proposed patch
Amende proposed patch
Signed-off patch
Bug 6885 Allow superlib to delete items from any library
Follow up patch
Bug 6885 Follow up patch, invert tested condition
Bug 6885 Follow up patch, invert tested condition

Description Frédéric Demians 2011-09-19 17:59:59 UTC
When IndependantBranches syspref is enabled, a 'regular' user can only delete items belonging to his/her library. But a superlibrarian should have the permission to delete items from all libraries. He can't for the time being.
Comment 1 Frédéric Demians 2011-09-19 18:12:26 UTC Comment hidden (obsolete)
Comment 2 Michael Hafen 2011-09-20 15:26:06 UTC
Looking at the patch (on the mailing list) userenv->{flags} != 1 isn't the best way to check for superlibrarian status.  I think that userenv->{flags} %2 != 1 is the preferred way.
Comment 3 Julian Maurice 2011-10-07 13:25:19 UTC
I confirm that, if user has more than the superlibrarian permission checked, userenv->{flags} will be greater than 1
Comment 4 Frédéric Demians 2011-10-07 13:40:06 UTC Comment hidden (obsolete)
Comment 5 Owen Leonard 2011-10-11 14:34:58 UTC Comment hidden (obsolete)
Comment 6 Paul Poulain 2011-10-12 15:07:50 UTC Comment hidden (obsolete)
Comment 7 Paul Poulain 2011-10-12 15:08:54 UTC
QA comment
  * good description
  * small fix, written like everywhere in the code to check against superlibrarian permission

=> passed QA
Comment 8 Ian Walls 2011-10-12 15:30:49 UTC
This will work to add the permissions check, yes, and it is consistent with how superlibrarian is tested in other parts of the code.  This method (modulo 2) is not universal, however, and another bug report should be filed to apply a uniform checking methodology across the entire codebase.  I recommend bitwise AND.

Passed QA.
Comment 9 Frédéric Demians 2011-10-12 15:55:18 UTC
(In reply to comment #8)
> This will work to add the permissions check, yes, and it is consistent
> with how superlibrarian is tested in other parts of the code.  This
> method (modulo 2) is not universal, however, and another bug report
> should be filed to apply a uniform checking methodology across the
> entire codebase.  I recommend bitwise AND.

Yes, you're correct, and there are a lot of locations in Koha where
permissions flags are not properly tested. A 'grep -R flags' show them.
The superlibrarian permission should be tested like this:

  if $flags & 1 

cataloguing: $flags & 4

and so on...
Comment 10 Chris Cormack 2011-10-13 02:55:59 UTC
No changes to templates, pushed
Comment 11 Nicole C. Engard 2011-11-01 15:46:51 UTC
I just tested this on Koha 3.6 and was unable to delete and item even though I was logged in as a superlibrarian ... can anyone confirm that this is or isn't fixed?
Comment 12 Owen Leonard 2011-11-01 16:33:22 UTC
(In reply to comment #11)
> I just tested this on Koha 3.6 and was unable to delete and item even though I
> was logged in as a superlibrarian

Unable to delete because the link wasn't there, or because of an error? It's working for me.
Comment 13 Nicole C. Engard 2011-11-02 00:27:57 UTC
I clicked DELETE (the link was there) and then it said I couldn't delete because it wasn't at my branch.
Comment 14 Frédéric Demians 2011-11-02 07:25:24 UTC
The patch is included in HEAD/3.6. If you are on a git install, you can check:

  git log --grep="Bug 6885"
Comment 15 Frédéric Demians 2012-01-04 17:13:39 UTC
This bug is back in HEAD/3.6.x. This is due to a follow up patch which
commit id is:

cdd798ec3216c6019adccc40935e93736ce4dc3b

The condition has to be inverted.
Comment 16 Frédéric Demians 2012-01-04 17:15:18 UTC Comment hidden (obsolete)
Comment 17 Chris Cormack 2012-01-05 02:30:48 UTC Comment hidden (obsolete)
Comment 18 Chris Cormack 2012-01-05 02:31:29 UTC
Created attachment 7047 [details] [review]
Bug 6885 Follow up patch, invert tested condition

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Not sure the brackets are nessecary, but I think I remember learning
bitwise operators have a lower priority than other relational
operators so wanted to make sure we notted the result of the bitwise
and, not the flag
Comment 19 Frédéric Demians 2012-01-05 08:53:30 UTC
> Not sure the brackets are nessecary, but I think I remember learning
> bitwise operators have a lower priority than other relational
> operators so wanted to make sure we notted the result of the bitwise
> and, not the flag

That's a wise decision. Thanks. And it adds readability.
Comment 20 Paul Poulain 2012-01-17 17:06:54 UTC
QA comment: tiny patch, nothing to say (except the condition is a little bit hard to read now, but could if be different ? I'm not sure)