diff --git a/C4/SIP/SIPServer.pm b/C4/SIP/SIPServer.pm index 98ba01006d..c4fbdecef6 100644 --- a/C4/SIP/SIPServer.pm +++ b/C4/SIP/SIPServer.pm @@ -236,6 +236,7 @@ sub process_request { Koha::Caches->flush_L1_caches(); $self->{account} = undef; # Clear out the account from the last request, it may be different + $self->{ils} = undef; # And the ILS handle, so a reused worker can't reuse a prior session's connection $self->{logger} = set_logger( Koha::Logger->get( { interface => 'sip' } ) ); # Flush previous MDCs to prevent accidentally leaking incorrect MDC-entries @@ -400,11 +401,10 @@ sub telnet_transport { if ( exists( $config->{accounts}->{$uid} ) && ( $pwd eq $config->{accounts}->{$uid}->{password} ) ) + && C4::SIP::Sip::MsgType::login_core( $self, $uid, $pwd ) ) { $account = $config->{accounts}->{$uid}; - if ( C4::SIP::Sip::MsgType::login_core( $self, $uid, $pwd ) ) { - last; - } + last; } siplog( "LOG_WARNING", "Invalid login attempt: '%s'", ( $uid || '' ) ); print("Invalid login$CRLF"); diff --git a/C4/SIP/Sip/MsgType.pm b/C4/SIP/Sip/MsgType.pm index 576520d49c..32fed467c3 100644 --- a/C4/SIP/Sip/MsgType.pm +++ b/C4/SIP/Sip/MsgType.pm @@ -927,6 +927,13 @@ sub login_core { my $uid = shift; my $pwd = shift; my $status = 1; # Assume it all works + + # Clear any session state up front, including anything left over from a previous connection + # on this preforked worker, so no login_core exit path can leave the caller acting as an + # authenticated terminal. Only a successful api_auth below re-establishes the session. + delete $server->{$_} foreach qw( account ils institution policy sip_username sip_password ); + + if ( !exists( $server->{config}->{accounts}->{$uid} ) ) { siplog( "LOG_WARNING", "MsgType::login_core: Unknown login '$uid'" ); $status = 0; @@ -935,13 +942,8 @@ sub login_core { $status = 0; } else { - # Store the active account someplace handy for everybody else to find. - $server->{account} = $server->{config}->{accounts}->{$uid}; - my $inst = $server->{account}->{institution}; - $server->{institution} = $server->{config}->{institutions}->{$inst}; - $server->{policy} = $server->{institution}->{policy}; - $server->{sip_username} = $uid; - $server->{sip_password} = $pwd; + # Authenticate before storing the account on the session + my $inst = $server->{config}->{accounts}->{$uid}->{institution}; my $auth_status = api_auth( $uid, $pwd, $inst ); if ( !$auth_status or $auth_status !~ /^ok$/i ) { @@ -951,6 +953,13 @@ sub login_core { ); $status = 0; } else { + # Store the active account configuration now user has authenticated + $server->{account} = $server->{config}->{accounts}->{$uid}; + $server->{institution} = $server->{config}->{institutions}->{$inst}; + $server->{policy} = $server->{institution}->{policy}; + $server->{sip_username} = $uid; + $server->{sip_password} = $pwd; + siplog( "LOG_INFO", "Successful login/auth for '%s' of '%s'", $server->{account}->{id}, $inst ); # diff --git a/t/db_dependent/SIP/MsgType.t b/t/db_dependent/SIP/MsgType.t index 9cab05b3ba..31ef482a21 100755 --- a/t/db_dependent/SIP/MsgType.t +++ b/t/db_dependent/SIP/MsgType.t @@ -19,13 +19,14 @@ use Modern::Perl; -use Test::More tests => 3; +use Test::More tests => 4; use Test::NoWarnings; use Test::Warn; use Test::MockModule; use C4::SIP::Sip::MsgType; use C4::SIP::Sip::Constants qw(:all); +use UNIVERSAL::require; use t::lib::TestBuilder; use t::lib::Mocks; @@ -349,3 +350,62 @@ subtest 'message parsing arrayref prevention' => sub { $schema->storage->txn_rollback; }; + +subtest 'login_core() authentication tests' => sub { + plan tests => 12; + + # A failed login must never leave the connection in an authenticated state. + my $uid = 'sip_bug_42847'; + + my $build_server = sub { + return { + config => { + accounts => { $uid => { id => $uid, institution => 'CPL' } }, + institutions => { CPL => { id => 'CPL', implementation => 'ILS', policy => {} } }, + }, + }; + }; + + my $auth_status = 'ok'; + my $mock_auth = Test::MockModule->new('C4::SIP::Sip::MsgType'); + $mock_auth->mock( 'api_auth', sub { return $auth_status; } ); + + my $mock_ils = Test::MockModule->new('C4::SIP::ILS'); + $mock_ils->mock( 'new', sub { return bless {}, 'C4::SIP::ILS'; } ); + + # Successful login stores the account and an ILS handle + $auth_status = 'ok'; + my $server = $build_server->(); + ok( C4::SIP::Sip::MsgType::login_core( $server, $uid, 'goodpw' ), 'login_core returns true on successful auth' ); + is( $server->{account}->{id}, $uid, 'account stored on the session after a successful login' ); + isa_ok( $server->{ils}, 'C4::SIP::ILS', 'ILS handle built after a successful login' ); + + # A wrong password must not store the account + $auth_status = 'failed'; + $server = $build_server->(); + ok( !C4::SIP::Sip::MsgType::login_core( $server, $uid, 'badpw' ), 'login_core returns false on failed auth' ); + is( $server->{account}, undef, 'account is not stored on the session after a failed login' ); + is( $server->{ils}, undef, 'no ILS handle after a failed login' ); + + # A reused prefork worker that handled a successful login must not let the + # next connection inherit that session when its login fails + $auth_status = 'ok'; + $server = $build_server->(); + C4::SIP::Sip::MsgType::login_core( $server, $uid, 'goodpw' ); # previous session on this worker + $auth_status = 'failed'; + ok( !C4::SIP::Sip::MsgType::login_core( $server, $uid, 'badpw' ), 'failed login on a reused worker returns false' ); + is( $server->{account}, undef, 'leftover account cleared after a failed login on a reused worker' ); + is( $server->{ils}, undef, 'leftover ILS handle cleared after a failed login on a reused worker' ); + + # A reused worker whose next connection presents an unknown terminal id must + # also have the leftover session cleared, not just the wrong-password case + $auth_status = 'ok'; + $server = $build_server->(); + C4::SIP::Sip::MsgType::login_core( $server, $uid, 'goodpw' ); # previous session on this worker + ok( + !C4::SIP::Sip::MsgType::login_core( $server, 'unknown_terminal', 'badpw' ), + 'login for an unknown terminal returns false' + ); + is( $server->{account}, undef, 'leftover account cleared after an unknown-terminal login on a reused worker' ); + is( $server->{ils}, undef, 'leftover ILS handle cleared after an unknown-terminal login on a reused worker' ); +};