Bug 36034 - cas_ticket is set to serialized patron object in session
Summary: cas_ticket is set to serialized patron object in session
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: Main
Hardware: All All
: P1 - high blocker (vote)
Assignee: Andreas Jonsson
QA Contact: David Cook
URL:
Keywords:
Depends on: 34893
Blocks:
  Show dependency treegraph
 
Reported: 2024-02-08 10:46 UTC by Andreas Jonsson
Modified: 2024-02-29 08:14 UTC (History)
9 users (show)

See Also:
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


Attachments
Bug 36034: fix capture of return values from checkpw (2.06 KB, patch)
2024-02-08 11:20 UTC, Andreas Jonsson
Details | Diff | Splinter Review
Bug 36034: fix capture of return values from checkpw (2.12 KB, patch)
2024-02-08 14:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 36034: fix capture of return values from checkpw (2.17 KB, patch)
2024-02-14 02:11 UTC, David Cook
Details | Diff | Splinter Review
Bug 36034: (bug 34893 follow-up) fix capture of return values from checkpw (2.21 KB, patch)
2024-02-14 07:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36034: Add test (2.23 KB, patch)
2024-02-14 07:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36034: [22.05] Add test (2.13 KB, patch)
2024-02-28 07:52 UTC, Jonathan Druart
Details | Diff | Splinter Review

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