Bug 25858 - Borrower permissions are broken by update from bug 22868
Summary: Borrower permissions are broken by update from bug 22868
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P1 - high blocker (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on: 22868
Blocks:
  Show dependency treegraph
 
Reported: 2020-06-23 19:12 UTC by Nick Clemens
Modified: 2020-07-22 22:10 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.02, 19.11.08, 19.05.13


Attachments
Bug 25858: Don't attempt to set flag if already set (942 bytes, patch)
2020-06-24 10:07 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25858: Don't attempt to set flag if already set (1006 bytes, patch)
2020-06-25 10:09 UTC, didier
Details | Diff | Splinter Review
Bug 25858: Don't attempt to set flag if already set (1.04 KB, patch)
2020-06-26 07:01 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 25858: Use bitwise OR for setting a bit in borrowers.flag (1.05 KB, patch)
2020-06-26 07:01 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2020-06-23 19:12:16 UTC
The update in bug 22868 flips bit 12 in flags on, however, it was backported and there is no check that the flag is off.

A borrower who had bit 11 will now have 4096 added twice which will break their permissions
Comment 1 Nick Clemens 2020-06-24 10:07:20 UTC
Created attachment 106240 [details] [review]
Bug 25858: Don't attempt to set flag if already set
Comment 2 didier 2020-06-25 09:47:36 UTC
Hi

Why not use & | and ^ ?

UPDATE borrowers SET flags = flags | (1<<12) WHERE flags & (1 << 11)

Regards
Didier
Comment 3 Nick Clemens 2020-06-25 10:01:33 UTC
(In reply to didier from comment #2)
> Hi
> 
> Why not use & | and ^ ?
> 
> UPDATE borrowers SET flags = flags | (1<<12) WHERE flags & (1 << 11)
> 
> Regards
> Didier

That is much more concise, I am not sure if it is clearer, but I am happy to have either solution, we just need to get this fixed
Comment 4 didier 2020-06-25 10:09:52 UTC
Created attachment 106282 [details] [review]
Bug 25858: Don't attempt to set flag if already set

Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com>
Comment 5 Julian Maurice 2020-06-26 06:52:46 UTC
(In reply to Nick Clemens from comment #3)
> (In reply to didier from comment #2)
> > Hi
> > 
> > Why not use & | and ^ ?
> > 
> > UPDATE borrowers SET flags = flags | (1<<12) WHERE flags & (1 << 11)
> > 
> > Regards
> > Didier
> 
> That is much more concise, I am not sure if it is clearer, but I am happy to
> have either solution, we just need to get this fixed

I confirm both solutions work, but in my opinion Didier's solution is more clear about what the intention is. I'll provide a followup
Comment 6 Julian Maurice 2020-06-26 07:01:07 UTC
Created attachment 106314 [details] [review]
Bug 25858: Don't attempt to set flag if already set

Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 7 Julian Maurice 2020-06-26 07:01:12 UTC
Created attachment 106315 [details] [review]
Bug 25858: Use bitwise OR for setting a bit in borrowers.flag

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Thanks-to: Didier Gautheron <didier.gautheron@biblibre.com>
Comment 8 Jonathan Druart 2020-06-29 09:54:50 UTC
Looks like it is impossible to add a new update DB entry to fix this problem :-/
Comment 9 Jonathan Druart 2020-06-29 11:50:54 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 10 Lucas Gass 2020-07-13 15:58:01 UTC
backported to 20.05.x for 20.05.02
Comment 11 Aleisha Amohia 2020-07-16 04:25:51 UTC
backported to 19.11.x for 19.11.08
Comment 12 Victor Grousset/tuxayo 2020-07-22 22:10:19 UTC
Backported to 19.05.x branch for 19.05.13