From 8af5e7d52fb59f64be1c818775008fae633780b4 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Tue, 11 Oct 2016 14:35:48 +0100 Subject: [PATCH] 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. --- C4/Auth.pm | 43 +++++++++++-------- C4/InstallAuth.pm | 18 +++++--- circ/circulation.pl | 2 +- cpanfile | 4 +- .../en/modules/admin/preferences/admin.pref | 1 - members/routing-lists.pl | 1 - tools/background-job-progress.pl | 1 - 7 files changed, 39 insertions(+), 31 deletions(-) diff --git a/C4/Auth.pm b/C4/Auth.pm index f281def2ce0..c7c1b91ba97 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -24,7 +24,7 @@ use Carp qw/croak/; use Digest::MD5 qw(md5_base64); use JSON qw/encode_json/; use URI::Escape; -use CGI::Session; +use Data::Session; require Exporter; use C4::Context; @@ -1232,7 +1232,7 @@ sub checkauth { $session->param( 'desk_name', $desk_name); $session->param( 'flags', $userflags ); $session->param( 'emailaddress', $emailaddress ); - $session->param( 'ip', $session->remote_addr() ); + $session->param( 'ip', $ENV{REMOTE_ADDR} ); $session->param( 'lasttime', time() ); $session->param( 'interface', $type); $session->param( 'shibboleth', $shibSuccess ); @@ -1261,7 +1261,7 @@ sub checkauth { C4::Context->_unset_userenv($sessionID); } $session->param( 'lasttime', time() ); - $session->param( 'ip', $session->remote_addr() ); + $session->param( 'ip', $ENV{REMOTE_ADDR} ); $session->param( 'sessiontype', 'anon' ); $session->param( 'interface', $type); } @@ -1273,7 +1273,7 @@ sub checkauth { $debug and warn "Initiating an anonymous session..."; # setting a couple of other session vars... - $session->param( 'ip', $session->remote_addr() ); + $session->param( 'ip', $ENV{REMOTE_ADDR} ); $session->param( 'lasttime', time() ); $session->param( 'sessiontype', 'anon' ); $session->param( 'interface', $type); @@ -1686,7 +1686,7 @@ sub check_api_auth { $session->param( 'branchname', $branchname ); $session->param( 'flags', $userflags ); $session->param( 'emailaddress', $emailaddress ); - $session->param( 'ip', $session->remote_addr() ); + $session->param( 'ip', $ENV{REMOTE_ADDR} ); $session->param( 'lasttime', time() ); $session->param( 'interface', 'api' ); } @@ -1833,10 +1833,9 @@ sub check_cookie_auth { =head2 get_session - use CGI::Session; my $session = get_session($sessionID); -Given a session ID, retrieve the CGI::Session object used to store +Given a session ID, retrieve the Data::Session object used to store the session's state. The session object can be used to store data that needs to be accessed by different scripts during a user's session. @@ -1850,28 +1849,34 @@ sub _get_session_params { my $storage_method = C4::Context->preference('SessionStorage'); if ( $storage_method eq 'mysql' ) { my $dbh = C4::Context->dbh; - return { dsn => "driver:MySQL;id:md5", dsn_args => { Handle => $dbh } }; - } - elsif ( $storage_method eq 'Pg' ) { - my $dbh = C4::Context->dbh; - return { dsn => "driver:PostgreSQL;id:md5", dsn_args => { Handle => $dbh } }; - } - elsif ( $storage_method eq 'memcached' && Koha::Caches->get_instance->memcached_cache ) { - my $memcached = Koha::Caches->get_instance()->memcached_cache; - return { dsn => "driver:memcached;id:md5", dsn_args => { Memcached => $memcached } }; + # Note that the doc is wrong, type is required, if not provided it default to File + return { dbh => $dbh, type => 'driver:mysql;id:MD5' }; + } elsif ( $storage_method eq 'Pg' ) { + die "The system preference SessionStorage is set to 'Pg' but this is not supported yet!"; + } elsif ( $storage_method eq 'memcached' and my $memcached = Koha::Caches->get_instance->memcached_cache ) { + return { + type => 'driver:Memcached;id:SHA1;serialize:DataDumper', + cache => $memcached, + }; + } elsif ( $storage_method ne 'tmp' ) { + warn "The system preference SessionStorage is set to an invalid value '$storage_method'"; } else { # catch all defaults to tmp should work on all systems + if ( $storage_method ne 'tmp' ) { + warn "The session has not been correctly retrieved, defaulting to temporary storage: " . $Data::Session::errstr; + } my $dir = C4::Context::temporary_directory; - my $instance = C4::Context->config( 'database' ); #actually for packages not exactly the instance name, but generally safer to leave it as it is - return { dsn => "driver:File;id:md5", dsn_args => { Directory => "$dir/cgisess_$instance" } }; + my $instance = C4::Context->config('database'); #actually for packages not exactly the instance name, but generally safer to leave it as it is + return { type => 'driver:File;id:MD5', directory => "$dir/cgisess_$instance" }; } } sub get_session { my $sessionID = shift; my $params = _get_session_params(); - return CGI::Session->new( $params->{dsn}, $sessionID, $params->{dsn_args} ); + my $session = Data::Session->new( %$params ) || die $Data::Session::errstr; + return $session; } diff --git a/C4/InstallAuth.pm b/C4/InstallAuth.pm index c3f5adcaa13..db73f2cef3e 100644 --- a/C4/InstallAuth.pm +++ b/C4/InstallAuth.pm @@ -19,7 +19,7 @@ package C4::InstallAuth; use Modern::Perl; use Digest::MD5 qw(md5_base64); -use CGI::Session; +use Data::Session; use File::Spec; require Exporter; @@ -240,11 +240,13 @@ sub checkauth { my %info; my ( $userid, $cookie, $sessionID, $flags, $envcookie ); my $logout = $query->param('logout.x'); + my $dir = File::Spec->tmpdir; if ( $sessionID = $query->cookie("CGISESSID") ) { C4::Context->_new_userenv($sessionID); - my $session = - CGI::Session->new( "driver:File", $sessionID, - { Directory => $sessdir } ); + my $session = Data::Session->new( + type => 'driver:File;id:MD5', + directory => $dir + ) or die $Data::Session::errstr; if ( $session->param('cardnumber') ) { C4::Context->set_userenv( $session->param('number'), @@ -282,8 +284,10 @@ sub checkauth { } } unless ($userid) { - my $session = - CGI::Session->new( "driver:File", undef, { Directory => $sessdir } ); + my $session = Data::Session->new( + type => 'driver:File;id:MD5', + directory => $dir + ) or die $Data::Session::errstr; $sessionID = $session->id; $userid = $query->param('userid'); C4::Context->_new_userenv($sessionID); @@ -325,7 +329,7 @@ sub checkauth { $session->param( 'flags', 1 ); $session->param( 'emailaddress', C4::Context->preference('KohaAdminEmailAddress') ); - $session->param( 'ip', $session->remote_addr() ); + $session->param( 'ip', $ENV{REMOTE_ADDR} ); $session->param( 'lasttime', time() ); $userid = C4::Context->config('user'); } diff --git a/circ/circulation.pl b/circ/circulation.pl index f6dd318f971..2da81ccf919 100755 --- a/circ/circulation.pl +++ b/circ/circulation.pl @@ -41,7 +41,7 @@ use MARC::Record; use C4::Reserves; use Koha::Holds; use C4::Context; -use CGI::Session; +use Data::Session; use Koha::AuthorisedValues; use Koha::CsvProfiles; use Koha::Patrons; diff --git a/cpanfile b/cpanfile index 6e13f049320..1ac3b62f945 100644 --- a/cpanfile +++ b/cpanfile @@ -9,7 +9,6 @@ requires 'CGI', '3.15'; requires 'CGI::Carp', '1.29'; requires 'CGI::Compile', '>= 0.17, != 0.24'; requires 'CGI::Emulate::PSGI', '0.20'; -requires 'CGI::Session', '4.2'; requires 'CPAN::Meta', '2.150006'; requires 'Cache::Memcached', '1.30'; requires 'Cache::Memcached::Fast::Safe', '0.06'; @@ -25,6 +24,9 @@ requires 'DBIx::Class::Schema::Loader', '0.07039'; requires 'DBIx::RunSQL', '0.14'; requires 'Data::Dumper', '2.121'; requires 'Data::ICal', '0.13'; +requires 'Data::Session', '1.18'; +requires 'Data::Session::Driver::Memcached', '1.18'; +requires 'Data::Session::Driver::mysql', '1.18'; requires 'Date::Calc', '5.4'; requires 'Date::Manip', '5.44'; requires 'DateTime', '0.58'; diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref index a858b1d0da5..7f3d071efb0 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref @@ -105,7 +105,6 @@ Administration: default: mysql choices: mysql: MySQL database - Pg: PostgreSQL database (not supported) tmp: Temporary files memcached: Memcached server - diff --git a/members/routing-lists.pl b/members/routing-lists.pl index dee5845a0dd..d72f53e74f7 100755 --- a/members/routing-lists.pl +++ b/members/routing-lists.pl @@ -25,7 +25,6 @@ use C4::Members; use C4::Context; use C4::Serials; use Koha::Patrons; -use CGI::Session; my $query = CGI->new; diff --git a/tools/background-job-progress.pl b/tools/background-job-progress.pl index 625389f9029..b9d48f2f3df 100755 --- a/tools/background-job-progress.pl +++ b/tools/background-job-progress.pl @@ -22,7 +22,6 @@ use Modern::Perl; # standard or CPAN modules used use IO::File; use CGI qw ( -utf8 ); -use CGI::Session; use C4::Context; use C4::Auth qw/check_cookie_auth/; use C4::BackgroundJob; -- 2.20.1