Bug 36034

Summary: cas_ticket is set to serialized patron object in session
Product: Koha Reporter: Andreas Jonsson <andreas.jonsson>
Component: AuthenticationAssignee: Andreas Jonsson <andreas.jonsson>
Status: Pushed to oldstable --- QA Contact: David Cook <dcook>
Severity: blocker    
Priority: P1 - high CC: dcook, dpavlin, f.demians, fridolin.somers, jonathan.druart, kyle, lucas, mtj
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34893
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.03,23.05.09,22.11.15,22.05.19
Circulation function:
Bug Depends on: 34893    
Bug Blocks:    
Attachments: Bug 36034: fix capture of return values from checkpw
Bug 36034: fix capture of return values from checkpw
Bug 36034: fix capture of return values from checkpw
Bug 36034: (bug 34893 follow-up) fix capture of return values from checkpw
Bug 36034: Add test
Bug 36034: [22.05] Add test

Description Andreas Jonsson 2024-02-08 10:46:26 UTC
The fix for bug 34893 alters the return value type of checkpw, but does not adequately update the code everywhere where this is used.  In particular, with shibboleth enabled the cas_ticket is sent to a patron object which gets serialized into the user session which quickly consumes a large amount of disk space on a busy system.
Comment 1 Andreas Jonsson 2024-02-08 11:20:07 UTC
Created attachment 161888 [details] [review]
Bug 36034: fix capture of return values from checkpw

Adapt code to the change of return value type of checkpw
introduced in bug 34893
Comment 2 Andreas Jonsson 2024-02-08 11:24:43 UTC
The disk space problem happens with binary logging enabled and using mysql as session storage.

Also, it seems to happen both with and without shibboleth enabled.

I have been running 23.05.08 for less than 2 days and are already seeing a large increase in disk space on several servers.

I have emergency patched my servers and I suspect that many libraries are going to run into problems with disk space in the coming weeks.
Comment 3 Kyle M Hall (khall) 2024-02-08 11:28:34 UTC
(In reply to Andreas Jonsson from comment #2)
> The disk space problem happens with binary logging enabled and using mysql
> as session storage.
> 
> Also, it seems to happen both with and without shibboleth enabled.
> 
> I have been running 23.05.08 for less than 2 days and are already seeing a
> large increase in disk space on several servers.
> 
> I have emergency patched my servers and I suspect that many libraries are
> going to run into problems with disk space in the coming weeks.

Is this patch ready for sign-off? That status is still NEW.
Comment 4 Andreas Jonsson 2024-02-08 11:34:53 UTC
It is complex code.  I'll let someone else be the judge if it is ready for signoff.

I have no good idea for test plan other than inspecting the binary logs of the database. 

But I would say that it is urgent.
Comment 5 Andreas Jonsson 2024-02-08 11:35:50 UTC
Also I am already running this patch live on several servers.
Comment 6 Kyle M Hall (khall) 2024-02-08 14:54:58 UTC
Created attachment 161926 [details] [review]
Bug 36034: fix capture of return values from checkpw

Adapt code to the change of return value type of checkpw
introduced in bug 34893

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 7 Kyle M Hall (khall) 2024-02-08 14:55:50 UTC
Changes seem clearly correct, all unit tests are passing.
Comment 8 David Cook 2024-02-14 02:11:42 UTC
Created attachment 162130 [details] [review]
Bug 36034: fix capture of return values from checkpw

Adapt code to the change of return value type of checkpw
introduced in bug 34893

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 9 David Cook 2024-02-14 02:16:59 UTC
As Kyle said, code review shows the fix clearly. 

You can also see the difference in the "sessions" table after a logging both before and after applying the patch. After applying the patch, the session data looks correct and isn't full of serialized data from a Koha::Patron object. 

QA tools are happy. 

This one is a ticking timebomb for anyone running one of the versions that contains bug 34893, so I think releasing this ASAP would be wise.

I don't know how to reproduce the "Too many nested open parens in regex;" error thrown by YAML::XS::__qr_loader, but I imagine it's happening to people in the wild as it's been happening to us in dev. 

Good job on reporting and fixing this one, Andreas. I didn't notice it in my emails until I sent that listserv email where you commented. Not sure what happened there...
Comment 10 Jonathan Druart 2024-02-14 07:54:11 UTC
Created attachment 162131 [details] [review]
Bug 36034: (bug 34893 follow-up) fix capture of return values from checkpw

Adapt code to the change of return value type of checkpw
introduced in bug 34893

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Jonathan Druart 2024-02-14 07:54:14 UTC
Created attachment 162132 [details] [review]
Bug 36034: Add test

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 12 Martin Renvoize (ashimema) 2024-02-14 10:33:55 UTC
Pushed to master for 24.05.00.

I did an inline tidy of the block added to unit tests to make the QA script happy.

Thanks Jonathan.
Comment 13 Fridolin Somers 2024-02-15 09:47:40 UTC
Pushed to 23.11.x for 23.11.03
Comment 14 Lucas Gass (lukeg) 2024-02-20 18:02:06 UTC
Backported to 23.05.x for upcoming 23.05.09
Comment 15 Frédéric Demians 2024-02-23 17:39:29 UTC
Backported 22.11.15
Comment 16 Jonathan Druart 2024-02-28 07:52:29 UTC
Created attachment 162529 [details] [review]
Bug 36034: [22.05] Add test

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>