CGI::Session is not maintained for ages and seems quite buggy. It would be the culprit of random logout problems. Another issue is the following crazy code in CGI::Session::Driver::DBI sub DESTROY { my $self = shift; unless ( defined $self->{Handle} && $self->{Handle} -> ping ) { $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away'); return; } unless ( $self->{Handle}->{AutoCommit} ) { $self->{Handle}->commit; } if ( $self->{_disconnect} ) { $self->{Handle}->disconnect; } } If AutoCommit is off, it will commit anyway... It causes some tests to fail (api) if SessionStorage is set to MySQL
Created attachment 56185 [details] [review] Bug 17427: Replace CGI::Session with Data::Session CGI::Session is not maintained for ages and seems quite buggy. It would be the culprit of random logout problems. Another issue is the following crazy code in CGI::Session::Driver::DBI sub DESTROY { my $self = shift; unless ( defined $self->{Handle} && $self->{Handle} -> ping ) { $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away'); return; } unless ( $self->{Handle}->{AutoCommit} ) { $self->{Handle}->commit; } if ( $self->{_disconnect} ) { $self->{Handle}->disconnect; } } If AutoCommit is off, it will commit anyway... It causes some tests to fail (api) if SessionStorage is set to MySQL This is just a POC and will only works with SessionStorage set to MySQL. Please test it and tell me if you find bug. If it looks bug free I will submit a follow-up to cover the other combinations of SessionStorage (temporary storage and Memcached)
This is great detective work, and it makes real sense to deprecate a deprecated session handler! I have tested with api and finally tests with autocommit set to null actually rolls back, so I no longer am left with a skewed db. please bring this on and I will happily test and sign off!
Created attachment 56504 [details] [review] Bug 17427: Replace CGI::Session with Data::Session CGI::Session is not maintained for ages and seems quite buggy. It would be the culprit of random logout problems. Another issue is the following crazy code in CGI::Session::Driver::DBI sub DESTROY { my $self = shift; unless ( defined $self->{Handle} && $self->{Handle} -> ping ) { $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away'); return; } unless ( $self->{Handle}->{AutoCommit} ) { $self->{Handle}->commit; } if ( $self->{_disconnect} ) { $self->{Handle}->disconnect; } } If AutoCommit is off, it will commit anyway... It causes some tests to fail (api) if SessionStorage is set to MySQL Note that PostgreSQL is not supported yet so no need to implement it. Test plan: 0/ The dependencies are not packaged for debian so far, so install it via cpan 1/ Test the 3 different values for SessionStorage 2/ Using different browsers (or sessions) login into Koha with different users. Naviguate => you should not be logged out 3/ Set SessionStorage to Memcached, stop memcached make sure that it defaults to file.
(In reply to Benjamin Rokseth from comment #2) > This is great detective work, and it makes real sense to deprecate a > deprecated session handler! I have tested with api and finally tests with > autocommit set to null actually rolls back, so I no longer am left with a > skewed db. > > please bring this on and I will happily test and sign off! It's your turn to play ;)
I should note: The first patch did not work as expected, it defaulted to File (instead of mysql).
Created attachment 56572 [details] [review] Bug 17427: Replace CGI::Session with Data::Session CGI::Session is not maintained for ages and seems quite buggy. It would be the culprit of random logout problems. Another issue is the following crazy code in CGI::Session::Driver::DBI sub DESTROY { my $self = shift; unless ( defined $self->{Handle} && $self->{Handle} -> ping ) { $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away'); return; } unless ( $self->{Handle}->{AutoCommit} ) { $self->{Handle}->commit; } if ( $self->{_disconnect} ) { $self->{Handle}->disconnect; } } If AutoCommit is off, it will commit anyway... It causes some tests to fail (api) if SessionStorage is set to MySQL Note that PostgreSQL is not supported yet so no need to implement it. Test plan: 0/ The dependencies are not packaged for debian so far, so install it via cpan 1/ Test the 3 different values for SessionStorage 2/ Using different browsers (or sessions) login into Koha with different users. Naviguate => you should not be logged out 3/ Set SessionStorage to Memcached, stop memcached make sure that it defaults to file. Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Works perfectly! Corrected a small typo in the PerlDependencies. No sessions lost in tempfiles, mysql or memcached, and they survived a plack restart.
Comment on attachment 56572 [details] [review] Bug 17427: Replace CGI::Session with Data::Session Review of attachment 56572 [details] [review]: ----------------------------------------------------------------- Generally looks good.. minor question about using ENV though.. not sure about that part of the change. ::: C4/Auth.pm @@ +1111,4 @@ > $session->param( 'branchname', $branchname ); > $session->param( 'flags', $userflags ); > $session->param( 'emailaddress', $emailaddress ); > + $session->param( 'ip', $ENV{REMOTE_ADDR} ); Did we test this against plack? Can we really rely on ENV for remote_addr... and in fact.. do we not compare the session ip to the env remote_addr in places as a security check?
(In reply to Martin Renvoize from comment #8) > Comment on attachment 56572 [details] [review] [review] > Bug 17427: Replace CGI::Session with Data::Session > > Review of attachment 56572 [details] [review] [review]: > ----------------------------------------------------------------- > > Generally looks good.. minor question about using ENV though.. not sure > about that part of the change. > > ::: C4/Auth.pm > @@ +1111,4 @@ > > $session->param( 'branchname', $branchname ); > > $session->param( 'flags', $userflags ); > > $session->param( 'emailaddress', $emailaddress ); > > + $session->param( 'ip', $ENV{REMOTE_ADDR} ); > > Did we test this against plack? Can we really rely on ENV for > remote_addr... and in fact.. do we not compare the session ip to the env > remote_addr in places as a security check? $ENV{REMOTE_ADDR} is correctly filled on each request by the ReverseProxy middleware as far as I recall (I debugged plack+ENV several days before making the packages plack integration official).
Please rebase. I'd say this one depends only on Mirko's answer about packaging Data::Session.
Created attachment 57067 [details] [review] Bug 17427: Replace CGI::Session with Data::Session CGI::Session is not maintained for ages and seems quite buggy. It would be the culprit of random logout problems. Another issue is the following crazy code in CGI::Session::Driver::DBI sub DESTROY { my $self = shift; unless ( defined $self->{Handle} && $self->{Handle} -> ping ) { $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away'); return; } unless ( $self->{Handle}->{AutoCommit} ) { $self->{Handle}->commit; } if ( $self->{_disconnect} ) { $self->{Handle}->disconnect; } } If AutoCommit is off, it will commit anyway... It causes some tests to fail (api) if SessionStorage is set to MySQL Note that PostgreSQL is not supported yet so no need to implement it. Test plan: 0/ The dependencies are not packaged for debian so far, so install it via cpan 1/ Test the 3 different values for SessionStorage 2/ Using different browsers (or sessions) login into Koha with different users. Naviguate => you should not be logged out 3/ Set SessionStorage to Memcached, stop memcached make sure that it defaults to file. Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
I do not recommend to push this patch into the next stable release (at least not at the beginning).
Created attachment 60355 [details] [review] Bug 17427: Replace CGI::Session with Data::Session CGI::Session is not maintained for ages and seems quite buggy. It would be the culprit of random logout problems. Another issue is the following crazy code in CGI::Session::Driver::DBI sub DESTROY { my $self = shift; unless ( defined $self->{Handle} && $self->{Handle} -> ping ) { $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away'); return; } unless ( $self->{Handle}->{AutoCommit} ) { $self->{Handle}->commit; } if ( $self->{_disconnect} ) { $self->{Handle}->disconnect; } } If AutoCommit is off, it will commit anyway... It causes some tests to fail (api) if SessionStorage is set to MySQL Note that PostgreSQL is not supported yet so no need to implement it. Test plan: 0/ The dependencies are not packaged for debian so far, so install it via cpan 1/ Test the 3 different values for SessionStorage 2/ Using different browsers (or sessions) login into Koha with different users. Naviguate => you should not be logged out 3/ Set SessionStorage to Memcached, stop memcached make sure that it defaults to file. Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Just a rebase...
QA: Looking here now
(In reply to Benjamin Rokseth from comment #7) > Works perfectly! Corrected a small typo in the PerlDependencies. > > No sessions lost in tempfiles, mysql or memcached, and they survived a plack > restart. Benjamin: What browsers did you test?
This seems to break scripts like stage-marc-import and tools/upload.pl. I am still testing a bit but after successfully uploading one file, I am not able to upload anymore. Also saw this error: stage-marc-import.pl: YAML::Tiny does not support C4::BackgroundJob references at /usr/local/share/perl/5.20.2/Data/Session/Serialize/YAML.pm line 18., Suspect that all scripts using BackgroungJob have issues now. Note that these jobs append information to the session.
Data::Session creates a CGI object. And we do not like that in scripts like upload-file.pl that call check_cookie_auth (calling get_session). The created CGI object in this case makes the upload disappear. Other scripts may have issues too going the similar cookie_auth way..
(In reply to Marcel de Rooy from comment #17) > This seems to break scripts like stage-marc-import and tools/upload.pl. > I am still testing a bit but after successfully uploading one file, I am not > able to upload anymore. > Also saw this error: > stage-marc-import.pl: YAML::Tiny does not support C4::BackgroundJob > references at /usr/local/share/perl/5.20.2/Data/Session/Serialize/YAML.pm > line 18., That can be fixed using DataDumper or FreezeThaw instead of YAML as serializer. (In reply to Marcel de Rooy from comment #18) > Data::Session creates a CGI object. > And we do not like that in scripts like upload-file.pl that call > check_cookie_auth (calling get_session). > The created CGI object in this case makes the upload disappear. > Other scripts may have issues too going the similar cookie_auth way.. I do not know how to fix that.
Created attachment 121337 [details] [review] Bug 17427: Replace CGI::Session with Data::Session CGI::Session is not maintained for ages and seems quite buggy. It would be the culprit of random logout problems. Another issue is the following crazy code in CGI::Session::Driver::DBI sub DESTROY { my $self = shift; unless ( defined $self->{Handle} && $self->{Handle} -> ping ) { $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away'); return; } unless ( $self->{Handle}->{AutoCommit} ) { $self->{Handle}->commit; } if ( $self->{_disconnect} ) { $self->{Handle}->disconnect; } } If AutoCommit is off, it will commit anyway... It causes some tests to fail (api) if SessionStorage is set to MySQL Note that PostgreSQL is not supported yet so no need to implement it. Test plan: 0/ The dependencies are not packaged for debian so far, so install it via cpan 1/ Test the 3 different values for SessionStorage 2/ Using different browsers (or sessions) login into Koha with different users. Naviguate => you should not be logged out 3/ Set SessionStorage to Memcached, stop memcached make sure that it defaults to file.
(patch rebased)
(In reply to Jonathan Druart from comment #21) > (patch rebased) Should this be "Needs Signoff" now?
(In reply to David Cook from comment #22) > (In reply to Jonathan Druart from comment #21) > > (patch rebased) > > Should this be "Needs Signoff" now? I rebased it to see if it fixed bug 28158, it doesn't. This patch can be tested, but I haven't much so far. In my opinion that's still one we need, maybe a good candidate for early 21.11 adoption? Feel free to test it!
Created attachment 121633 [details] [review] Bug 17427: Use JSON serialization
We are now aiming to provide a long-term solution for the encoding issue found on bug 28489. Using Data::Session using the JSON serializer seems to be the more robust solution. However there is still an issue if SessionStorage=tmp file Note that sessions.a_session must be longtext.
(In reply to Jonathan Druart from comment #25) > However there is still an issue if SessionStorage=tmp file root@kohadevbox$ more /var/lib/koha/kohadev/tmp/cgisess_koha_kohadev/cgisess_6967e16ccb3dddff1c32735b7930e967 {"_SESSION_CTIME":1622821220,"_SESSION_ETIME":0,"id":"koha","interface":"intranet","number":51,"ip":"172.20.0.1","lasttime":"1622821226","_SESSION_PTIME":{},"_SESSION_ATIME":1622821226,"flags":1,"surname":"koha" ,"cardnumber":"42","_SESSION_ID":"6967e16ccb3dddff1c32735b7930e967","branchname":"✔️ ❤️ ★","branch":"✔️❤️★"} root@kohadevbox$ file /var/lib/koha/kohadev/tmp/cgisess_koha_kohadev/cgisess_6967e16ccb3dddff1c32735b7930e967 /var/lib/koha/kohadev/tmp/cgisess_koha_kohadev/cgisess_6967e16ccb3dddff1c32735b7930e967: JSON data
Bug 28517 might be related to this?
JSON serialization does not work (when SessionStorage=tmp), selenium tests from bug 28489 are failing. With longblob: # Failed test 'logged-in-branch-name set - SessionStorage=mysql, branchname=Test2❤️' # at t/db_dependent/selenium/regressions.t line 306. # got: 'Test2â¤ï¸Â' # expected: 'Test2❤️' With longtext: # Failed test 'logged-in-branch-name set - SessionStorage=tmp, branchname=Test2❤️' # at t/db_dependent/selenium/regressions.t line 306. # got: 'Test2â¤ï¸' # expected: 'Test2❤️'
And it's not possible to add a serializer that is not in the same dir as others, because of this code in Data::Session: 318 sub get_my_serializers 319 { 320 my($self) = @_; 321 my($path) = $self -> _get_pm_path('Serialize'); 322 323 # Warning: Use sort map{} read_dir, not map{} sort read_dir. But, why? 324 325 my(@serializer) = sort map{s/.pm//; $_} read_dir($path); 326 327 ($#serializer < 0) && die __PACKAGE__ . '. No serializers available'; 328 329 ($self -> verbose > 1) && $self -> log('Serializers: ' . join(', ', @serializer) ); 330 331 $self -> my_serializers(\@serializer); 332 333 } # End of get_my_serializers.
(In reply to Jonathan Druart from comment #28) This comment was confusing, with JSON serialization we have: With longtext it's working with SessionStorage=mysql With longblob it's failing with mysql. But it does not work with SessionStorage=tmp.
Created attachment 121657 [details] [review] Bug 17427: Replace CGI::Session with Data::Session CGI::Session is not maintained for ages and seems quite buggy. It would be the culprit of random logout problems. Another issue is the following crazy code in CGI::Session::Driver::DBI sub DESTROY { my $self = shift; unless ( defined $self->{Handle} && $self->{Handle} -> ping ) { $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away'); return; } unless ( $self->{Handle}->{AutoCommit} ) { $self->{Handle}->commit; } if ( $self->{_disconnect} ) { $self->{Handle}->disconnect; } } If AutoCommit is off, it will commit anyway... It causes some tests to fail (api) if SessionStorage is set to MySQL Note that PostgreSQL is not supported yet so no need to implement it. Test plan: 0/ The dependencies are not packaged for debian so far, so install it via cpan 1/ Test the 3 different values for SessionStorage 2/ Using different browsers (or sessions) login into Koha with different users. Naviguate => you should not be logged out 3/ Set SessionStorage to Memcached, stop memcached make sure that it defaults to file.
Created attachment 121658 [details] [review] Bug 17427: Use JSON serialization
Created attachment 121659 [details] [review] Bug 17427: Use our own YAML::XS serializer
And, FWIW, if I move the YAML::XS serializer (see last patch) in the Data/Session/Serialize directory, all tests pass (longblob).
(In reply to Jonathan Druart from comment #34) > And, FWIW, if I move the YAML::XS serializer (see last patch) in the > Data/Session/Serialize directory, all tests pass (longblob). Does this have an unlisted dependency on Bug 28519? I'm just curious since the serializers are in a "lib" directory.
Yes David.
hi joubu i've uploaded a libdata-session-perl pkg to koha-staging/dev, for testing root@kohadevbox:/kohadevbox# apt install libdata-session-perl Reading package lists... Done Building dependency tree Reading state information... Done The following additional packages will be installed: libautovivification-perl libdbd-sqlite3-perl libdbix-admin-createtable-perl libfile-slurper-perl libhash-fieldhash-perl libperlio-utf8-strict-perl The following NEW packages will be installed: libautovivification-perl libdata-session-perl libdbd-sqlite3-perl libdbix-admin-createtable-perl libfile-slurper-perl libhash-fieldhash-perl libperlio-utf8-strict-perl 0 upgraded, 7 newly installed, 0 to remove and 2 not upgraded. Need to get 376 kB of archives. root@kohadevbox:/kohadevbox# apt-cache policy libdata-session-perl libdata-session-perl: Installed: 1.18-1 Candidate: 1.18-1 Version table: *** 1.18-1 500 500 http://debian.koha-community.org/koha-staging dev/main amd64 Packages 100 /var/lib/dpkg/status
(In reply to Jonathan Druart from comment #34) > And, FWIW, if I move the YAML::XS serializer (see last patch) in the > Data/Session/Serialize directory, all tests pass (longblob). Wait this works despite https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17427#c29 ?
(In reply to David Cook from comment #38) > (In reply to Jonathan Druart from comment #34) > > And, FWIW, if I move the YAML::XS serializer (see last patch) in the > > Data/Session/Serialize directory, all tests pass (longblob). > > Wait this works despite > https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17427#c29 ? Oh maybe I misunderstand what you're saying... you meant in the vendor perl not the Koha perl lib... right?
(In reply to David Cook from comment #39) > (In reply to David Cook from comment #38) > > (In reply to Jonathan Druart from comment #34) > > > And, FWIW, if I move the YAML::XS serializer (see last patch) in the > > > Data/Session/Serialize directory, all tests pass (longblob). > > > > Wait this works despite > > https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17427#c29 ? > > Oh maybe I misunderstand what you're saying... you meant in the vendor perl > not the Koha perl lib... right? Yes, I copied it to /path/Data/Session/Serialize/yamlxs.pm where /path/Data/Session/ is `pmpath Data::Session`.
(In reply to Jonathan Druart from comment #40) > > Oh maybe I misunderstand what you're saying... you meant in the vendor perl > > not the Koha perl lib... right? > > Yes, I copied it to /path/Data/Session/Serialize/yamlxs.pm where > /path/Data/Session/ is `pmpath Data::Session`. But that's not a real solution for production. Shouldn't "Needs Signoff" be changed back to 'Assigned' or "In Discussion"?
Ok, let's discuss then. What do we do? I am ok with having our module where Data::Session is expecting it.
(In reply to Jonathan Druart from comment #42) > Ok, let's discuss then. What do we do? > > I am ok with having our module where Data::Session is expecting it. How do you propose to do that in production though? The only reasonable option I see would be creating our Data::Session::Serializer::yamlxs as a standalone library (with its own git repository), and then packaging that and storing it in the Koha Community APT repo, and then adding it as a Koha dependency.
What about https://metacpan.org/pod/MojoX::Session
(In reply to Tomás Cohen Arazi from comment #44) > What about https://metacpan.org/pod/MojoX::Session It hasn't been updated in over 7 years and the developer says he doesn't want to develop it anymore (although he said he was open to pull requests): https://github.com/vti/mojox-session/issues/19 But then there's been a pull request sitting for over 8 years too https://github.com/vti/mojox-session/pull/14
Marcel suggests writing Koha::Session at https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29942#c2 Maybe we should just do that. Jonathan opened this issue nearly 6 years ago and we still haven't resolved it...
(In reply to David Cook from comment #43) > (In reply to Jonathan Druart from comment #42) > > Ok, let's discuss then. What do we do? > > > > I am ok with having our module where Data::Session is expecting it. > > How do you propose to do that in production though? > > The only reasonable option I see would be creating our > Data::Session::Serializer::yamlxs as a standalone library (with its own git > repository), and then packaging that and storing it in the Koha Community > APT repo, and then adding it as a Koha dependency. I don't understand. Why isn't this ok for production? It's what we do already for CGI::Session
Created attachment 150569 [details] [review] Bug 17427: Replace CGI::Session with Data::Session CGI::Session is not maintained for ages and seems quite buggy. It would be the culprit of random logout problems. Another issue is the following crazy code in CGI::Session::Driver::DBI sub DESTROY { my $self = shift; unless ( defined $self->{Handle} && $self->{Handle} -> ping ) { $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away'); return; } unless ( $self->{Handle}->{AutoCommit} ) { $self->{Handle}->commit; } if ( $self->{_disconnect} ) { $self->{Handle}->disconnect; } } If AutoCommit is off, it will commit anyway... It causes some tests to fail (api) if SessionStorage is set to MySQL Note that PostgreSQL is not supported yet so no need to implement it. Test plan: 0/ The dependencies are not packaged for debian so far, so install it via cpan 1/ Test the 3 different values for SessionStorage 2/ Using different browsers (or sessions) login into Koha with different users. Naviguate => you should not be logged out 3/ Set SessionStorage to Memcached, stop memcached make sure that it defaults to file.
Created attachment 150570 [details] [review] Bug 17427: Use JSON serialization
Created attachment 150571 [details] [review] Bug 17427: Use our own YAML::XS serializer
Patches rebased, status back to NSO. Why isn't this ready? What am I missing? Everything is working correctly in my tests.
*** Bug 29942 has been marked as a duplicate of this bug. ***
This needs more work to be ready for push: * Adjust deps (cpanfile) * `git grep CGI::Session` occurrences to be adjusted * Remove FIXMEs from api tests But please test and tell me if it's worth spending time to continue here.
(In reply to Jonathan Druart from comment #53) > This needs more work to be ready for push: > * Adjust deps (cpanfile) > * `git grep CGI::Session` occurrences to be adjusted > * Remove FIXMEs from api tests > > But please test and tell me if it's worth spending time to continue here. I seems worthwhile to me if only because CGI::Session is unmaintained!
(In reply to Jonathan Druart from comment #53) > This needs more work to be ready for push: > * Adjust deps (cpanfile) > * `git grep CGI::Session` occurrences to be adjusted > * Remove FIXMEs from api tests > > But please test and tell me if it's worth spending time to continue here. +1
I'm getting the error "Data::Session. Unknown serialize 'yamlxs' at /usr/local/share/perl/5.32.1/Data/Session.pm line 752". I installed Data::Session, are there other deps I missed?
(In reply to Kyle M Hall from comment #56) > I'm getting the error "Data::Session. Unknown serialize 'yamlxs' at > /usr/local/share/perl/5.32.1/Data/Session.pm line 752". > > I installed Data::Session, are there other deps I missed? Yes, you need to copy the lib file cp lib/Data/Session/Serialize/yamlxs.pm /usr/share/perl5/Data/Session/Serialize/yamlxs.pm
Can we (In reply to Jonathan Druart from comment #57) > (In reply to Kyle M Hall from comment #56) > > I'm getting the error "Data::Session. Unknown serialize 'yamlxs' at > > /usr/local/share/perl/5.32.1/Data/Session.pm line 752". > > > > I installed Data::Session, are there other deps I missed? > > Yes, you need to copy the lib file > cp lib/Data/Session/Serialize/yamlxs.pm > /usr/share/perl5/Data/Session/Serialize/yamlxs.pm Can we ship Data::Session inside the Koha project instead, and get rid of the need for that?
No, please read the previous comments.
(In reply to Jonathan Druart from comment #59) > No, please read the previous comments. Well, yes, we could, if we embed Data::Session and modify it. But what would we do that? Isn't the "lib trick" already in place anyway?
(In reply to Jonathan Druart from comment #59) > No, please read the previous comments. Well, yes, we could, if we embed Data::Session and modify it. But why would we do that? Isn't the "lib trick" already in place anyway?
Created attachment 150603 [details] [review] Bug 17427: Replace CGI::Session with Data::Session CGI::Session is not maintained for ages and seems quite buggy. It would be the culprit of random logout problems. Another issue is the following crazy code in CGI::Session::Driver::DBI sub DESTROY { my $self = shift; unless ( defined $self->{Handle} && $self->{Handle} -> ping ) { $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away'); return; } unless ( $self->{Handle}->{AutoCommit} ) { $self->{Handle}->commit; } if ( $self->{_disconnect} ) { $self->{Handle}->disconnect; } } If AutoCommit is off, it will commit anyway... It causes some tests to fail (api) if SessionStorage is set to MySQL Note that PostgreSQL is not supported yet so no need to implement it. Test plan: 0/ The dependencies are not packaged for debian so far, so install it via cpan 1/ Test the 3 different values for SessionStorage 2/ Using different browsers (or sessions) login into Koha with different users. Naviguate => you should not be logged out 3/ Set SessionStorage to Memcached, stop memcached make sure that it defaults to file. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 150604 [details] [review] Bug 17427: Use JSON serialization Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 150605 [details] [review] Bug 17427: Use our own YAML::XS serializer Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 150606 [details] [review] Bug 17427: Remove use lines for CGI::Session from Auth.pm Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
The big blocker is replacing the use of CGI::Session::Find (https://metacpan.org/pod/CGI::Session#find(-%5C&code-)) in C4::Auth_with_cas
(In reply to Jonathan Druart from comment #61) > (In reply to Jonathan Druart from comment #59) > > No, please read the previous comments. > > Well, yes, we could, if we embed Data::Session and modify it. But why would > we do that? > > Isn't the "lib trick" already in place anyway? I am wrong. The existing "lib trick" is copying our 'lib' dir content to /usr/share/koha/lib but we need yamlxs.pm in /usr/share/perl5/Data/Session/Serialize/
Created attachment 150633 [details] [review] Bug 17427: Remove more occurrences of CGI::Session
Created attachment 150634 [details] [review] Bug 17427: Remove SessionStorage mock from tests
Created attachment 150635 [details] [review] Bug 17427: Embed Data::Session Test plan: Install libdata-session-perl and make sure our lib has priority over the installed version
Created attachment 150636 [details] [review] Bug 17427: Remove confusion between session id and param id FIXME Certainly more work needed here
Created attachment 150637 [details] [review] Bug 17427: HACKY - force new session Look at Data::Session->new, it's calling $self -> load_session; Then look at load_session then user_id => we are retrieving the ID from the cookie But in our Auth.t tests we are mocking CGI->cookie to always return the previous CGISESSID We should certainly adjust the test here, and remove this patch. 194 # Note: We can test return values from checkauth here since we mocked the safe_exit after the Redirect 303 195 is( $return[0], $patron2->userid, 'Login of patron2 approved' ); 196 isnt( $return[2], $sessionID, 'Did not return previous session ID' ); 197 ok( $return[2], 'New session ID not empty' );
Created attachment 150638 [details] [review] Bug 17427: Retrieve sessionID from HTTP_COOKIE To make some tests pass (t/db_dependent/Auth.t) and mimick what did CGI::Session, but that is certainly useless and the tests should be adjusted.
Created attachment 150639 [details] [review] Bug 17427: Fix id vs userid in tests
still 2 tests failing /kohadevbox/koha/t/db_dependent/Auth.t .. 12/17 # Failed test at /kohadevbox/koha/t/db_dependent/Auth.t line 840. # got: 'anon' # expected: 'expired' # Failed test at /kohadevbox/koha/t/db_dependent/Auth.t line 841. # got: 'Data::Session=HASH(0x564c72143498)' # expected: undef # Looks like you failed 2 tests of 31. /kohadevbox/koha/t/db_dependent/Auth.t .. 15/17 # Failed test 'checkauth & check_cookie_auth' # at /kohadevbox/koha/t/db_dependent/Auth.t line 909.
(In reply to Kyle M Hall from comment #66) > The big blocker is replacing the use of CGI::Session::Find > (https://metacpan.org/pod/CGI::Session#find(-%5C&code-)) in C4::Auth_with_cas Yes, that's a tricky one. And really ugly. First the method is experimental, and then we are looping over all the sessions to retrieve the corresponding one... outch! We could eventually generate the session ID from the CAS ticket (using Koha::Encryption for instance?). Would that work? I am not familiar with CAS!
I am done for now, please help (review, feedback, test, see last commit messages)!
After applying the patches and running reset_all in ktd, I get the following error when I try to log in: Data::Session::Driver::mysql::store(): DBI Exception: DBD::mysql::db do failed: Data too long for column 'id' at row 1 at /kohadevbox/koha/lib/Data/Session.pm line 262 at /usr/share/perl5/DBIx/Class/Exception.pm line 77
Created attachment 150926 [details] [review] Bug 17427: Remove SessionStorage mock from tests
Created attachment 150927 [details] [review] Bug 17427: Embed Data::Session Test plan: Install libdata-session-perl and make sure our lib has priority over the installed version
Created attachment 150928 [details] [review] Bug 17427: Remove confusion between session id and param id FIXME Certainly more work needed here
Created attachment 150929 [details] [review] Bug 17427: HACKY - force new session Look at Data::Session->new, it's calling $self -> load_session; Then look at load_session then user_id => we are retrieving the ID from the cookie But in our Auth.t tests we are mocking CGI->cookie to always return the previous CGISESSID We should certainly adjust the test here, and remove this patch. 194 # Note: We can test return values from checkauth here since we mocked the safe_exit after the Redirect 303 195 is( $return[0], $patron2->userid, 'Login of patron2 approved' ); 196 isnt( $return[2], $sessionID, 'Did not return previous session ID' ); 197 ok( $return[2], 'New session ID not empty' );
Created attachment 150930 [details] [review] Bug 17427: Retrieve sessionID from HTTP_COOKIE To make some tests pass (t/db_dependent/Auth.t) and mimick what did CGI::Session, but that is certainly useless and the tests should be adjusted. Commit modified, added ';' see comment 78 - if ( $http_cookie && $http_cookie =~ m{CGISESSID=([^,:]*)} ) { + if ( $http_cookie && $http_cookie =~ m{CGISESSID=([^,:;]*)} ) { sessionID was "a035bf508448b8b3ecd1ca23609b90b3; bib_list=; KohaOpacLanguage=; JWT= "
Created attachment 150931 [details] [review] Bug 17427: Fix id vs userid in tests
(In reply to Kyle M Hall from comment #78) > After applying the patches and running reset_all in ktd, I get the following > error when I try to log in: > > Data::Session::Driver::mysql::store(): DBI Exception: DBD::mysql::db do > failed: Data too long for column 'id' at row 1 at > /kohadevbox/koha/lib/Data/Session.pm line 262 > at /usr/share/perl5/DBIx/Class/Exception.pm line 77 Fixed in "Retrieve sessionID from HTTP_COOKIE" Patches rebased against master.
(In reply to Jonathan Druart from comment #73) > Created attachment 150638 [details] [review] [review] > Bug 17427: Retrieve sessionID from HTTP_COOKIE > > To make some tests pass (t/db_dependent/Auth.t) and mimick what did > CGI::Session, but that is certainly useless and the tests should be > adjusted. This patch fixes some tests but make the sessionID handling very wrong. We always end up with the same sessionID (even after logout, change user, etc.) I think we should revert it and remove the related tests.
(In reply to Mason James from comment #37) > hi joubu > i've uploaded a libdata-session-perl pkg to koha-staging/dev, for testing If we are going with the approach we have now we won't need it.
(In reply to Jonathan Druart from comment #80) > Created attachment 150927 [details] [review] [review] > Bug 17427: Embed Data::Session > > Test plan: > Install libdata-session-perl and make sure our lib has priority > over the installed version An alternative could be to contribute Data::Session::Serialize::yamlxs. Ron seems pretty responsive on Github, and it looks like Mason has contributed a couple patches in Feb 2023.
What's outstanding on this one?
(In reply to David Cook from comment #89) > What's outstanding on this one? Read the commit messages.
(In reply to Jonathan Druart from comment #90) > (In reply to David Cook from comment #89) > > What's outstanding on this one? > > Read the commit messages. I did and I couldn't make sense of them.
(In reply to David Cook from comment #91) > (In reply to Jonathan Druart from comment #90) > > (In reply to David Cook from comment #89) > > > What's outstanding on this one? > > > > Read the commit messages. > > I did and I couldn't make sense of them. Ok, trying to be more verbose then. 1. For discussion, waiting for review and feedback: (In reply to Jonathan Druart from comment #82) > Created attachment 150929 [details] [review] [review] > Bug 17427: HACKY - force new session > > Look at Data::Session->new, it's calling $self -> load_session; > Then look at load_session then user_id > => we are retrieving the ID from the cookie > > But in our Auth.t tests we are mocking CGI->cookie to always return the > previous CGISESSID > > We should certainly adjust the test here, and remove this patch. > > 194 # Note: We can test return values from checkauth here since we > mocked the safe_exit after the Redirect 303 > 195 is( $return[0], $patron2->userid, 'Login of patron2 approved' ); > 196 isnt( $return[2], $sessionID, 'Did not return previous session > ID' ); > 197 ok( $return[2], 'New session ID not empty' ); What do you think about this, should we adjust the test? 2. The behaviour is buggy, see the following comment: (In reply to Jonathan Druart from comment #86) > (In reply to Jonathan Druart from comment #73) > > Created attachment 150638 [details] [review] [review] [review] > > Bug 17427: Retrieve sessionID from HTTP_COOKIE > > > > To make some tests pass (t/db_dependent/Auth.t) and mimick what did > > CGI::Session, but that is certainly useless and the tests should be > > adjusted. > > This patch fixes some tests but make the sessionID handling very wrong. We > always end up with the same sessionID (even after logout, change user, etc.) > > I think we should revert it and remove the related tests. Do you agree that we should remove the related tests? 3. This needs to be fully tested, but first we need to fix the failures and bugs highlighted in 1 and 2.
Thanks, Jonathan. I appreciate that. I think I'll have to look at the code as it's running, but it won't apply at the moment. I have some holidays this week and next, so I won't be around heaps, but if you get that rebased then I can take a look.
Created attachment 153030 [details] [review] Bug 17427: Replace CGI::Session with Data::Session CGI::Session is not maintained for ages and seems quite buggy. It would be the culprit of random logout problems. Another issue is the following crazy code in CGI::Session::Driver::DBI sub DESTROY { my $self = shift; unless ( defined $self->{Handle} && $self->{Handle} -> ping ) { $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away'); return; } unless ( $self->{Handle}->{AutoCommit} ) { $self->{Handle}->commit; } if ( $self->{_disconnect} ) { $self->{Handle}->disconnect; } } If AutoCommit is off, it will commit anyway... It causes some tests to fail (api) if SessionStorage is set to MySQL Note that PostgreSQL is not supported yet so no need to implement it. Test plan: 0/ The dependencies are not packaged for debian so far, so install it via cpan 1/ Test the 3 different values for SessionStorage 2/ Using different browsers (or sessions) login into Koha with different users. Naviguate => you should not be logged out 3/ Set SessionStorage to Memcached, stop memcached make sure that it defaults to file. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 153031 [details] [review] Bug 17427: Use JSON serialization Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 153032 [details] [review] Bug 17427: Use our own YAML::XS serializer Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 153033 [details] [review] Bug 17427: Remove use lines for CGI::Session from Auth.pm Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 153034 [details] [review] Bug 17427: Remove more occurrences of CGI::Session
Created attachment 153035 [details] [review] Bug 17427: Remove SessionStorage mock from tests
Created attachment 153036 [details] [review] Bug 17427: Embed Data::Session Test plan: Install libdata-session-perl and make sure our lib has priority over the installed version
Created attachment 153037 [details] [review] Bug 17427: Remove confusion between session id and param id FIXME Certainly more work needed here
Created attachment 153038 [details] [review] Bug 17427: HACKY - force new session Look at Data::Session->new, it's calling $self -> load_session; Then look at load_session then user_id => we are retrieving the ID from the cookie But in our Auth.t tests we are mocking CGI->cookie to always return the previous CGISESSID We should certainly adjust the test here, and remove this patch. 194 # Note: We can test return values from checkauth here since we mocked the safe_exit after the Redirect 303 195 is( $return[0], $patron2->userid, 'Login of patron2 approved' ); 196 isnt( $return[2], $sessionID, 'Did not return previous session ID' ); 197 ok( $return[2], 'New session ID not empty' );
Created attachment 153039 [details] [review] Bug 17427: Retrieve sessionID from HTTP_COOKIE To make some tests pass (t/db_dependent/Auth.t) and mimick what did CGI::Session, but that is certainly useless and the tests should be adjusted. Commit modified, added ';' see comment 78 - if ( $http_cookie && $http_cookie =~ m{CGISESSID=([^,:]*)} ) { + if ( $http_cookie && $http_cookie =~ m{CGISESSID=([^,:;]*)} ) { sessionID was "a035bf508448b8b3ecd1ca23609b90b3; bib_list=; KohaOpacLanguage=; JWT= "
Created attachment 153040 [details] [review] Bug 17427: Fix id vs userid in tests
(In reply to Jonathan Druart from comment #76) > (In reply to Kyle M Hall from comment #66) > > The big blocker is replacing the use of CGI::Session::Find > > (https://metacpan.org/pod/CGI::Session#find(-%5C&code-)) in C4::Auth_with_cas > > Yes, that's a tricky one. And really ugly. First the method is experimental, > and then we are looping over all the sessions to retrieve the corresponding > one... outch! > > We could eventually generate the session ID from the CAS ticket (using > Koha::Encryption for instance?). Would that work? I am not familiar with CAS! And I forgot this one, it is a blocker!
Hum, I had to install the following packages: apt install libautovivification-perl libhash-fieldhash-perl libfile-slurper-perl
(In reply to Jonathan Druart from comment #106) > Hum, I had to install the following packages: > apt install libautovivification-perl libhash-fieldhash-perl > libfile-slurper-perl Those are libdata-session-perl deps.