Bug 6885 - Superlibrarian users can't delete items from another library when IndependantBranches
Summary: Superlibrarian users can't delete items from another library when Independant...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: 3.6
Hardware: All All
: P3 normal (vote)
Assignee: Frédéric Demians
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-19 17:59 UTC by Frédéric Demians
Modified: 2013-12-05 19:57 UTC (History)
7 users (show)

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


Attachments
Proposed patch (1.63 KB, patch)
2011-09-19 18:12 UTC, Frédéric Demians
Details | Diff | Splinter Review
Amende proposed patch (1.63 KB, patch)
2011-10-07 13:40 UTC, Frédéric Demians
Details | Diff | Splinter Review
Signed-off patch (1.73 KB, patch)
2011-10-11 14:34 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 6885 Allow superlib to delete items from any library (1.73 KB, patch)
2011-10-12 15:07 UTC, Paul Poulain
Details | Diff | Splinter Review
Follow up patch (840 bytes, patch)
2012-01-04 17:15 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 6885 Follow up patch, invert tested condition (1.09 KB, patch)
2012-01-05 02:30 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 6885 Follow up patch, invert tested condition (1.09 KB, patch)
2012-01-05 02:31 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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)