From e094828905c89c3631456a17e49c047de9125b4b Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Thu, 20 Jan 2022 12:05:11 +0100 Subject: [PATCH 01/10] Bug 29915: Don't generate a new session ID for anonymous navigation When a user is not logged in, a new session ID is generated every time a new page is hit. Signed-off-by: Marcel de Rooy Signed-off-by: Nick Clemens Signed-off-by: Martin Renvoize Signed-off-by: Kyle M Hall --- C4/Auth.pm | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/C4/Auth.pm b/C4/Auth.pm index 650618bacd..65b897ac1c 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -925,14 +925,11 @@ sub checkauth { -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); - my $sessiontype = $session->param('sessiontype') || ''; - unless ( $sessiontype && $sessiontype eq 'anon' ) { #if this is an anonymous session, we want to update the session, but not behave as if they are logged in... - $flags = haspermission( $userid, $flagsrequired ); - if ($flags) { - $loggedin = 1; - } else { - $info{'nopermission'} = 1; - } + $flags = haspermission( $userid, $flagsrequired ); + if ($flags) { + $loggedin = 1; + } else { + $info{'nopermission'} = 1; } } } elsif ( !$logout ) { @@ -953,7 +950,7 @@ sub checkauth { unless ( $userid ) { #we initiate a session prior to checking for a username to allow for anonymous sessions... - my $session = get_session("") or die "Auth ERROR: Cannot get_session()"; + $session ||= get_session("") or die "Auth ERROR: Cannot get_session()"; # Save anonymous search history in new session so it can be retrieved # by get_template_and_user to store it in user's search history after From 97c9e2db9bce7c5b3c2f885ae714409bb41f3b3c Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Mon, 24 Jan 2022 13:08:01 +0000 Subject: [PATCH 02/10] Bug 29915: Changes for get_session and check_cookie_auth If we look for an existing session, do not create a new one. Found a bug in the unset_userenv calls. For this moment changing the calls in Auth here. Later fix goes to bug 29954. Signed-off-by: Marcel de Rooy Signed-off-by: Jonathan Druart Signed-off-by: Nick Clemens Signed-off-by: Martin Renvoize Signed-off-by: Kyle M Hall --- C4/Auth.pm | 65 +++++++++++++++++++++++++++++------------------------- 1 file changed, 35 insertions(+), 30 deletions(-) diff --git a/C4/Auth.pm b/C4/Auth.pm index 65b897ac1c..404e612ddc 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -23,6 +23,7 @@ use Carp qw( croak ); use Digest::MD5 qw( md5_base64 ); use CGI::Session; +use CGI::Session::ErrorHandler; use URI; use URI::QueryParam; @@ -897,7 +898,7 @@ sub checkauth { $anon_search_history = $session->param('search_history'); $session->delete(); $session->flush; - C4::Context->_unset_userenv($sessionID); + C4::Context::_unset_userenv($sessionID); } elsif ($logout) { @@ -906,7 +907,7 @@ sub checkauth { my $shibSuccess = C4::Context->userenv->{'shibboleth'}; $session->delete(); $session->flush; - C4::Context->_unset_userenv($sessionID); + C4::Context::_unset_userenv($sessionID); if ($cas and $caslogout) { logout_cas($query, $type); @@ -1099,7 +1100,7 @@ sub checkauth { } else { $info{'nopermission'} = 1; - C4::Context->_unset_userenv($sessionID); + C4::Context::_unset_userenv($sessionID); } my ( $borrowernumber, $firstname, $surname, $userflags, $branchcode, $branchname, $emailaddress, $desk_id, @@ -1224,7 +1225,7 @@ sub checkauth { else { if ($userid) { $info{'invalid_username_or_password'} = 1; - C4::Context->_unset_userenv($sessionID); + C4::Context::_unset_userenv($sessionID); } $session->param( 'lasttime', time() ); $session->param( 'ip', $session->remote_addr() ); @@ -1682,57 +1683,57 @@ sub check_cookie_auth { # however, if a userid parameter is present (i.e., from # a form submission, assume that any current cookie # is to be ignored - unless ( defined $sessionID and $sessionID ) { + unless ( $sessionID ) { return ( "failed", undef ); } + C4::Context::_unset_userenv($sessionID); # remove old userenv first my $session = get_session($sessionID); - C4::Context->_new_userenv($sessionID); if ($session) { - C4::Context->interface($session->param('interface')); - C4::Context->set_userenv( - $session->param('number'), $session->param('id') // '', - $session->param('cardnumber'), $session->param('firstname'), - $session->param('surname'), $session->param('branch'), - $session->param('branchname'), $session->param('flags'), - $session->param('emailaddress'), $session->param('shibboleth'), - $session->param('desk_id'), $session->param('desk_name'), - $session->param('register_id'), $session->param('register_name') - ); - my $userid = $session->param('id'); my $ip = $session->param('ip'); my $lasttime = $session->param('lasttime'); my $timeout = _timeout_syspref(); if ( !$lasttime || ( $lasttime < time() - $timeout ) ) { - # time out $session->delete(); $session->flush; - C4::Context->_unset_userenv($sessionID); return ("expired", undef); - } elsif ( C4::Context->preference('SessionRestrictionByIP') && $ip ne $remote_addr ) { + } elsif ( C4::Context->preference('SessionRestrictionByIP') && $ip ne $remote_addr ) { # IP address changed $session->delete(); $session->flush; - C4::Context->_unset_userenv($sessionID); return ( "restricted", undef, { old_ip => $ip, new_ip => $remote_addr}); + } elsif ( $userid ) { $session->param( 'lasttime', time() ); my $flags = defined($flagsrequired) ? haspermission( $userid, $flagsrequired ) : 1; if ($flags) { + C4::Context->_new_userenv($sessionID); + C4::Context->interface($session->param('interface')); + C4::Context->set_userenv( + $session->param('number'), $session->param('id') // '', + $session->param('cardnumber'), $session->param('firstname'), + $session->param('surname'), $session->param('branch'), + $session->param('branchname'), $session->param('flags'), + $session->param('emailaddress'), $session->param('shibboleth'), + $session->param('desk_id'), $session->param('desk_name'), + $session->param('register_id'), $session->param('register_name') + ); return ( "ok", $session ); + } else { + $session->delete(); + $session->flush; + return ( "failed", undef ); } + } else { + C4::Context->_new_userenv($sessionID); + C4::Context->interface($session->param('interface')); + C4::Context->set_userenv( undef, q{} ); return ( "anon", $session ); } - # If here user was logged in, but doesn't have correct permissions - # could be an 'else' at `if($flags) return "ok"` , but left here to catch any errors - $session->delete(); - $session->flush; - C4::Context->_unset_userenv($sessionID); - return ( "failed", undef ); } else { return ( "expired", undef ); } @@ -1778,9 +1779,13 @@ sub _get_session_params { sub get_session { my $sessionID = shift; my $params = _get_session_params(); - my $session = CGI::Session->new( $params->{dsn}, $sessionID, $params->{dsn_args} ); - if ( ! $session ){ - die CGI::Session->errstr(); + my $session; + if( $sessionID ) { # find existing + CGI::Session::ErrorHandler->set_error( q{} ); # clear error, cpan issue #111463 + $session = CGI::Session->load( $params->{dsn}, $sessionID, $params->{dsn_args} ); + } else { + $session = CGI::Session->new( $params->{dsn}, $sessionID, $params->{dsn_args} ); + # $session->flush; } return $session; } From 16e498a42eba089f3da9a23f6c91591d1071de56 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Tue, 25 Jan 2022 13:55:00 +0000 Subject: [PATCH 03/10] Bug 29915: Tiny session adjustments It may be that we need a few additional flushes. And checking the returned session before clearing busc. Signed-off-by: Marcel de Rooy Signed-off-by: Jonathan Druart Signed-off-by: Nick Clemens Signed-off-by: Martin Renvoize Signed-off-by: Kyle M Hall --- C4/Auth.pm | 11 +++++++---- C4/Search/History.pm | 1 + 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/C4/Auth.pm b/C4/Auth.pm index 404e612ddc..437293b4cb 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -561,9 +561,10 @@ sub get_template_and_user { unless ( $pagename =~ /^(?:MARC|ISBD)?detail$/ or $pagename =~ /^showmarc$/ or $pagename =~ /^addbybiblionumber$/ - or $pagename =~ /^review$/ ) { - my $sessionSearch = get_session( $sessionID || $in->{'query'}->cookie("CGISESSID") ); - $sessionSearch->clear( ["busc"] ) if ( $sessionSearch->param("busc") ); + or $pagename =~ /^review$/ ) + { + my $sessionSearch = get_session( $sessionID ); + $sessionSearch->clear( ["busc"] ) if $sessionSearch; } } @@ -899,6 +900,7 @@ sub checkauth { $session->delete(); $session->flush; C4::Context::_unset_userenv($sessionID); + $sessionID = undef; } elsif ($logout) { @@ -908,6 +910,7 @@ sub checkauth { $session->delete(); $session->flush; C4::Context::_unset_userenv($sessionID); + $sessionID = undef; if ($cas and $caslogout) { logout_cas($query, $type); @@ -945,7 +948,6 @@ sub checkauth { } unless ( $loggedin ) { - $sessionID = undef; $userid = undef; } @@ -1243,6 +1245,7 @@ sub checkauth { $session->param( 'sessiontype', 'anon' ); $session->param( 'interface', $type); } + $session->flush; } # END unless ($userid) # finished authentification, now respond diff --git a/C4/Search/History.pm b/C4/Search/History.pm index df36cb0fbc..fec4f004fd 100644 --- a/C4/Search/History.pm +++ b/C4/Search/History.pm @@ -218,6 +218,7 @@ sub set_to_session { return () unless $session; $session->param( 'search_history', uri_escape( encode_json($search_history) ) ); + $session->flush; } 1; From 690a25d7ccb5dc28d05045246cd85c9e093826bc Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Tue, 25 Jan 2022 14:52:31 +0000 Subject: [PATCH 04/10] Bug 29915: (QA follow-up) Fix POD typo Signed-off-by: Marcel de Rooy Signed-off-by: Jonathan Druart Signed-off-by: Nick Clemens Signed-off-by: Martin Renvoize Signed-off-by: Kyle M Hall --- C4/Auth.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/C4/Auth.pm b/C4/Auth.pm index 437293b4cb..3bd6689031 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -1623,7 +1623,7 @@ sub check_api_auth { =head2 check_cookie_auth - ($status, $sessionId) = check_api_auth($cookie, $userflags); + ($status, $sessionId) = check_cookie_auth($cookie, $userflags); Given a CGISESSID cookie set during a previous login to Koha, determine if the user has the privileges specified by C<$userflags>. C<$userflags> From 1050d70d7f262ed657fbe34d4cc6e2fcab5ab575 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Wed, 26 Jan 2022 11:58:33 +0100 Subject: [PATCH 05/10] Bug 29915: Add tests Signed-off-by: Marcel de Rooy Signed-off-by: Jonathan Druart Signed-off-by: Nick Clemens Signed-off-by: Martin Renvoize Signed-off-by: Kyle M Hall --- t/db_dependent/Auth.t | 122 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 119 insertions(+), 3 deletions(-) diff --git a/t/db_dependent/Auth.t b/t/db_dependent/Auth.t index f053bd115d..58879ed28e 100755 --- a/t/db_dependent/Auth.t +++ b/t/db_dependent/Auth.t @@ -10,11 +10,12 @@ use CGI qw ( -utf8 ); use Test::MockObject; use Test::MockModule; use List::MoreUtils qw/all any none/; -use Test::More tests => 24; +use Test::More tests => 14; use Test::Warn; use t::lib::Mocks; use t::lib::TestBuilder; +use C4::Auth; use C4::Members; use Koha::AuthUtils qw/hash_password/; use Koha::Database; @@ -201,7 +202,7 @@ subtest 'checkpw lockout tests' => sub { # get_template_and_user tests -{ # Tests for the language URL parameter +subtest 'get_template_and_user' => sub { # Tests for the language URL parameter sub MockedCheckauth { my ($query,$authnotrequired,$flagsrequired,$type) = @_; @@ -342,7 +343,9 @@ subtest 'checkpw lockout tests' => sub { ); is($template->{VARS}->{'opac_name'}, "multibranch-19", "Opac name was set correctly"); is($template->{VARS}->{'opac_search_limit'}, "branch:multibranch-19", "Search limit was set correctly"); -} + + delete $ENV{"HTTP_COOKIE"}; +}; # Check that there is always an OPACBaseURL set. my $input = CGI->new(); @@ -479,4 +482,117 @@ subtest 'check_cookie_auth' => sub { #FIXME We should have a test to cover 'failed' status when a user has logged in, but doesn't have permission }; +subtest 'checkauth & check_cookie_auth' => sub { + plan tests => 27; + + # flags = 4 => { catalogue => 1 } + my $patron = $builder->build_object({ class => 'Koha::Patrons', value => { flags => 4 } }); + my $password = 'password'; + t::lib::Mocks::mock_preference( 'RequireStrongPassword', 0 ); + $patron->set_password( { password => $password } ); + + my $cgi_mock = Test::MockModule->new('CGI'); + $cgi_mock->mock( 'request_method', sub { return 'POST' } ); + + my $cgi = CGI->new; + + my $auth = Test::MockModule->new( 'C4::Auth' ); + # Tests will fail if we hit safe_exit + $auth->mock( 'safe_exit', sub { return } ); + + my ( $userid, $cookie, $sessionID, $flags ); + { + # checkauth will redirect and safe_exit if not authenticated and not authorized + local *STDOUT; + my $stdout; + open STDOUT, '>', \$stdout; + C4::Auth::checkauth($cgi, 0, {catalogue => 1}); + like( $stdout, qr{\s*Log in to your account} ); + $sessionID = ( $stdout =~ m{Set-Cookie: CGISESSID=((\d|\w)+);} ) ? $1 : undef; + ok($sessionID); + close STDOUT; + }; + + my $first_sessionID = $sessionID; + + $ENV{"HTTP_COOKIE"} = "CGISESSID=$sessionID"; + # Not authenticated yet, checkauth didn't return the session + ( $userid, $cookie, $sessionID, $flags ) = C4::Auth::checkauth($cgi, 0, {catalogue => 1}); + is( $sessionID, undef); + is( $userid, undef); + + # Same here, check_cookie_auth does not return the session + my ( $auth_status, $session ) = C4::Auth::check_cookie_auth($sessionID, {catalogue => 1}); + is( $auth_status, 'failed' ); + is( $session, undef ); + + # Simulating the login form submission + $cgi->param('userid', $patron->userid); + $cgi->param('password', $password); + + # Logged in! + ( $userid, $cookie, $sessionID, $flags ) = C4::Auth::checkauth($cgi, 0, {catalogue => 1}); + is( $sessionID, $first_sessionID ); + is( $userid, $patron->userid ); + + ( $auth_status, $session ) = C4::Auth::check_cookie_auth($sessionID, {catalogue => 1}); + is( $auth_status, 'ok' ); + is( $session->id, $first_sessionID ); + + # Logging out! + $cgi->param('logout.x', 1); + ( $userid, $cookie, $sessionID, $flags ) = C4::Auth::checkauth($cgi, 0, {catalogue => 1}); + is( $sessionID, undef ); + is( $ENV{"HTTP_COOKIE"}, "CGISESSID=$first_sessionID", 'HTTP_COOKIE not unset' ); + ( $auth_status, $session) = C4::Auth::check_cookie_auth($sessionID, {catalogue => 1}); + is( $auth_status, "failed"); + is( $session, undef ); + + { + # Trying to access without sessionID + undef $ENV{"HTTP_COOKIE"}; + $cgi = CGI->new; + ( $auth_status, $session) = C4::Auth::check_cookie_auth(undef, {catalogue => 1}); + is( $auth_status, 'failed' ); + is( $session, undef ); + + ( $userid, $cookie, $sessionID, $flags ) = C4::Auth::checkauth($cgi, 0, {catalogue => 1}); + is( $userid, undef ); + is( $sessionID, undef ); + + } + + { + # Hit unauthorized page then reuse the cookie + undef $ENV{"HTTP_COOKIE"}; + $cgi = CGI->new; + # Logging in + $cgi->param('userid', $patron->userid); + $cgi->param('password', $password); + ( $userid, $cookie, $sessionID, $flags ) = C4::Auth::checkauth($cgi, 0, {catalogue => 1}); + is( $userid, $patron->userid ); + $first_sessionID = $sessionID; + + $ENV{"HTTP_COOKIE"} = "CGISESSID=$sessionID"; + + # Patron does not have the borrowers permission + ( $userid, $cookie, $sessionID, $flags ) = C4::Auth::checkauth($cgi, 0, {borrowers => 1}); + is( $userid, undef ); + is( $sessionID, undef ); + + ( $auth_status, $session) = C4::Auth::check_cookie_auth($sessionID, {borrowers => 1}); + is( $auth_status, "failed" ); + is( $session, undef ); + + # Reuse the cookie + ( $userid, $cookie, $sessionID, $flags ) = C4::Auth::checkauth($cgi, 0, {catalogue => 1}); + is( $userid, $patron->userid ); + is( $sessionID, $first_sessionID ); + + ( $auth_status, $session) = C4::Auth::check_cookie_auth($sessionID, 0, {catalogue => 1}); + is( $auth_status, "ok" ); + is( $session, $first_sessionID ); + } +}; + $schema->storage->txn_rollback; From b4748d9ab5434950d2adc69c3e100e71489d50f6 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Date: Wed, 26 Jan 2022 12:27:56 +0000 Subject: [PATCH 06/10] Bug 29915: Changes to Auth.t Test plan: Run Auth.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> --- t/db_dependent/Auth.t | 98 +++++++++++++++++++++++++++++++++---------- 1 file changed, 76 insertions(+), 22 deletions(-) diff --git a/t/db_dependent/Auth.t b/t/db_dependent/Auth.t index 58879ed28e..6e82bd4182 100755 --- a/t/db_dependent/Auth.t +++ b/t/db_dependent/Auth.t @@ -10,7 +10,7 @@ use CGI qw ( -utf8 ); use Test::MockObject; use Test::MockModule; use List::MoreUtils qw/all any none/; -use Test::More tests => 14; +use Test::More tests => 15; use Test::Warn; use t::lib::Mocks; use t::lib::TestBuilder; @@ -483,7 +483,7 @@ subtest 'check_cookie_auth' => sub { }; subtest 'checkauth & check_cookie_auth' => sub { - plan tests => 27; + plan tests => 31; # flags = 4 => { catalogue => 1 } my $patron = $builder->build_object({ class => 'Koha::Patrons', value => { flags => 4 } }); @@ -517,12 +517,18 @@ subtest 'checkauth & check_cookie_auth' => sub { $ENV{"HTTP_COOKIE"} = "CGISESSID=$sessionID"; # Not authenticated yet, checkauth didn't return the session - ( $userid, $cookie, $sessionID, $flags ) = C4::Auth::checkauth($cgi, 0, {catalogue => 1}); + { + local *STDOUT; + my $stdout; + open STDOUT, '>', \$stdout; + ( $userid, $cookie, $sessionID, $flags ) = C4::Auth::checkauth($cgi, 0, {catalogue => 1} ); + close STDOUT; + } is( $sessionID, undef); is( $userid, undef); - # Same here, check_cookie_auth does not return the session - my ( $auth_status, $session ) = C4::Auth::check_cookie_auth($sessionID, {catalogue => 1}); + # Sending undefined fails obviously + my ( $auth_status, $session ) = C4::Auth::check_cookie_auth($sessionID, {catalogue => 1} ); is( $auth_status, 'failed' ); is( $session, undef ); @@ -544,55 +550,103 @@ subtest 'checkauth & check_cookie_auth' => sub { ( $userid, $cookie, $sessionID, $flags ) = C4::Auth::checkauth($cgi, 0, {catalogue => 1}); is( $sessionID, undef ); is( $ENV{"HTTP_COOKIE"}, "CGISESSID=$first_sessionID", 'HTTP_COOKIE not unset' ); - ( $auth_status, $session) = C4::Auth::check_cookie_auth($sessionID, {catalogue => 1}); - is( $auth_status, "failed"); + ( $auth_status, $session) = C4::Auth::check_cookie_auth( $first_sessionID, {catalogue => 1} ); + is( $auth_status, "expired"); is( $session, undef ); { # Trying to access without sessionID - undef $ENV{"HTTP_COOKIE"}; $cgi = CGI->new; ( $auth_status, $session) = C4::Auth::check_cookie_auth(undef, {catalogue => 1}); is( $auth_status, 'failed' ); is( $session, undef ); - ( $userid, $cookie, $sessionID, $flags ) = C4::Auth::checkauth($cgi, 0, {catalogue => 1}); + # This will fail on permissions + undef $ENV{"HTTP_COOKIE"}; + { + local *STDOUT; + my $stdout; + open STDOUT, '>', \$stdout; + ( $userid, $cookie, $sessionID, $flags ) = C4::Auth::checkauth($cgi, 0, {catalogue => 1} ); + close STDOUT; + } is( $userid, undef ); is( $sessionID, undef ); - } { - # Hit unauthorized page then reuse the cookie - undef $ENV{"HTTP_COOKIE"}; + # First logging in $cgi = CGI->new; - # Logging in $cgi->param('userid', $patron->userid); $cgi->param('password', $password); ( $userid, $cookie, $sessionID, $flags ) = C4::Auth::checkauth($cgi, 0, {catalogue => 1}); is( $userid, $patron->userid ); $first_sessionID = $sessionID; - $ENV{"HTTP_COOKIE"} = "CGISESSID=$sessionID"; - # Patron does not have the borrowers permission - ( $userid, $cookie, $sessionID, $flags ) = C4::Auth::checkauth($cgi, 0, {borrowers => 1}); + # $ENV{"HTTP_COOKIE"} = "CGISESSID=$sessionID"; # not needed, we use $cgi here + { + local *STDOUT; + my $stdout; + open STDOUT, '>', \$stdout; + ( $userid, $cookie, $sessionID, $flags ) = C4::Auth::checkauth($cgi, 0, {borrowers => 1} ); + close STDOUT; + } is( $userid, undef ); is( $sessionID, undef ); - ( $auth_status, $session) = C4::Auth::check_cookie_auth($sessionID, {borrowers => 1}); + # When calling check_cookie_auth, the session will be deleted + ( $auth_status, $session) = C4::Auth::check_cookie_auth( $first_sessionID, { borrowers => 1 } ); is( $auth_status, "failed" ); is( $session, undef ); + ( $auth_status, $session) = C4::Auth::check_cookie_auth( $first_sessionID, { borrowers => 1 } ); + is( $auth_status, 'expired', 'Session no longer exists' ); - # Reuse the cookie + # Try reusing the deleted session: since it does not exist, we should get a new one now when passing correct permissions + $cgi->cookie( -name => 'CGISESSID', value => $first_sessionID ); ( $userid, $cookie, $sessionID, $flags ) = C4::Auth::checkauth($cgi, 0, {catalogue => 1}); is( $userid, $patron->userid ); - is( $sessionID, $first_sessionID ); - - ( $auth_status, $session) = C4::Auth::check_cookie_auth($sessionID, 0, {catalogue => 1}); + isnt( $sessionID, undef, 'Check if we have a sessionID' ); + isnt( $sessionID, $first_sessionID, 'New value expected' ); + ( $auth_status, $session) = C4::Auth::check_cookie_auth( $sessionID, {catalogue => 1} ); is( $auth_status, "ok" ); - is( $session, $first_sessionID ); + is( $session->id, $sessionID, 'Same session' ); + # Two additional tests on userenv + is( $C4::Context::context->{activeuser}, $session->id, 'Check if environment has been setup for session' ); + is( C4::Context->userenv->{id}, $userid, 'Check userid in userenv' ); } }; +subtest 'Userenv clearing in check_cookie_auth' => sub { + # Note: We did already test userenv for a logged-in user in previous subtest + plan tests => 9; + + t::lib::Mocks::mock_preference( 'timeout', 600 ); + my $cgi = CGI->new; + + # Create a new anonymous session by passing a fake session ID + $cgi->cookie( -name => 'CGISESSID', -value => 'fake_sessionID' ); + my ($userid, $cookie, $sessionID, $flags ) = C4::Auth::checkauth($cgi, 1); + my ( $auth_status, $session) = C4::Auth::check_cookie_auth( $sessionID ); + is( $auth_status, 'anon', 'Should be anonymous' ); + is( $C4::Context::context->{activeuser}, $session->id, 'Check activeuser' ); + is( defined C4::Context->userenv, 1, 'There should be a userenv' ); + is( C4::Context->userenv->{id}, q{}, 'userid should be empty string' ); + + # Make the session expire now, check_cookie_auth will delete it + $session->param('lasttime', time() - 1200 ); + $session->flush; + ( $auth_status, $session) = C4::Auth::check_cookie_auth( $sessionID ); + is( $auth_status, 'expired', 'Should be expired' ); + is( C4::Context->userenv, undef, 'Environment should be cleared too' ); + + # Show that we clear the userenv again: set up env and check deleted session + C4::Context->_new_userenv( $sessionID ); + C4::Context->set_userenv; # empty + is( defined C4::Context->userenv, 1, 'There should be an empty userenv again' ); + ( $auth_status, $session) = C4::Auth::check_cookie_auth( $sessionID ); + is( $auth_status, 'expired', 'Should be expired already' ); + is( C4::Context->userenv, undef, 'Environment should be cleared again' ); +}; + $schema->storage->txn_rollback; From 1476c17cda5b973f1e07e7963d03023e61865890 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Date: Wed, 26 Jan 2022 15:54:28 +0000 Subject: [PATCH 07/10] Bug 29915: (QA follow-up) Add a comment in checkauth on $flags This is quite a misleading call. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> --- C4/Auth.pm | 1 + 1 file changed, 1 insertion(+) diff --git a/C4/Auth.pm b/C4/Auth.pm index 3bd6689031..94a8d750fc 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -879,6 +879,7 @@ sub checkauth { } elsif ( $sessionID = $query->cookie("CGISESSID") ) { # assignment, not comparison my ( $return, $more_info ); + # NOTE: $flags in the following call is still undefined ! ( $return, $session, $more_info ) = check_cookie_auth( $sessionID, $flags, { remote_addr => $ENV{REMOTE_ADDR}, skip_version_check => 1 } ); From 6235e8354c1d7ed8b28ea1cd13d89a9ba4321d4b Mon Sep 17 00:00:00 2001 From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Date: Thu, 27 Jan 2022 15:12:40 +0100 Subject: [PATCH 08/10] Bug 29915: Add a note for tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> --- t/db_dependent/Auth.t | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/t/db_dependent/Auth.t b/t/db_dependent/Auth.t index 6e82bd4182..5228254ab8 100755 --- a/t/db_dependent/Auth.t +++ b/t/db_dependent/Auth.t @@ -602,6 +602,11 @@ subtest 'checkauth & check_cookie_auth' => sub { ( $auth_status, $session) = C4::Auth::check_cookie_auth( $first_sessionID, { borrowers => 1 } ); is( $auth_status, 'expired', 'Session no longer exists' ); + # NOTE: It is not what the UI is doing. + # From the UI we are allowed to hit an unauthorized page then reuse the session to hit back authorized area. + # It is because check_cookie_auth is ALWAYS called from checkauth WITHOUT $flagsrequired + # It then return "ok", when the previous called got "failed" + # Try reusing the deleted session: since it does not exist, we should get a new one now when passing correct permissions $cgi->cookie( -name => 'CGISESSID', value => $first_sessionID ); ( $userid, $cookie, $sessionID, $flags ) = C4::Auth::checkauth($cgi, 0, {catalogue => 1}); From 59f1bcebaf90c73dcc41fb2645449a90a781db91 Mon Sep 17 00:00:00 2001 From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Date: Fri, 28 Jan 2022 17:12:04 +0100 Subject: [PATCH 09/10] Bug 29915: Add selenium tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> --- .../intranet-tmpl/prog/en/modules/auth.tt | 4 +- t/db_dependent/selenium/authentication.t | 130 +++++++++++++++++- 2 files changed, 129 insertions(+), 5 deletions(-) diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/auth.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/auth.tt index 73b3761764..499c5bdbd3 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/auth.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/auth.tt @@ -138,8 +138,8 @@ [% END %] [% IF ( nopermission ) %] - <p><a href="javascript:window.history.back()">[Previous page]</a> - <a href="/">[Main page]</a></p> + <p><a id="previous_page" href="javascript:window.history.back()">[Previous page]</a> + <a id="mainpage" href="/">[Main page]</a></p> [% END %] diff --git a/t/db_dependent/selenium/authentication.t b/t/db_dependent/selenium/authentication.t index 591fb09d07..bd86fa7796 100755 --- a/t/db_dependent/selenium/authentication.t +++ b/t/db_dependent/selenium/authentication.t @@ -3,7 +3,7 @@ # This file is part of Koha. # # Copyright (C) 2017 Catalyst IT -# Copyright 2018 Koha Development team +# Copyright 2021 Koha Development team # # Koha is free software; you can redistribute it and/or modify it # under the terms of the GNU General Public License as published by @@ -42,7 +42,7 @@ SKIP: { my $driver = $s->driver; subtest 'Staff interface authentication' => sub { - plan tests => 5; + plan tests => 6; my $mainpage = $s->base_url . q|mainpage.pl|; $driver->get($mainpage); like( $driver->get_title, qr(Log in to Koha), 'Hitting the main page should redirect to the login form'); @@ -68,11 +68,74 @@ SKIP: { $s->auth( $patron->userid, $password ); like( $driver->get_title, qr(Koha staff interface), 'Patron with flags superlibrarian should be able to login' ); + subtest 'not authorized' => sub { + plan tests => 17; + + # First, logout! + $driver->get($mainpage . q|?logout.x=1|); + $patron->flags(4)->store; # Patron has only catalogue permission + like( $driver->get_title, qr(Log in to Koha), 'Patron should hit the login form after logout' ); + # Login! + $s->fill_form({ userid => $patron->userid, password => $password }); + $s->driver->find_element('//input[@id="submit-button"]')->click; + + my $cookie = $driver->get_cookie_named('CGISESSID'); + my $first_sessionID = $cookie->{value}; + + # Patron is logged in and got a CGISESSID cookie, miam + like( $driver->get_title, qr(Koha staff interface), 'Patron is logged in' ); + $cookie = $driver->get_cookie_named('CGISESSID'); + is( $cookie->{value}, $first_sessionID, 'no new session after login, the session has been upgraded' ); + + # Authorized page can be accessed, cookie does not change + $driver->get( $s->base_url . q|catalogue/search.pl| ); + like( $driver->get_title, qr(Advanced search), 'Patron can access advanced search' ); + $cookie = $driver->get_cookie_named('CGISESSID'); + is( $cookie->{value}, $first_sessionID, 'no new session after hit' ); + + # Unauthorized page redirect to the login form + $driver->get( $s->base_url . q|circ/circulation.pl| ); + like( $driver->get_title, qr(Access denied), 'Patron cannot access the circulation module' ); + # But the patron does not lose the CGISESSID cookie! + $cookie = $driver->get_cookie_named('CGISESSID'); + is( $cookie->{value}, $first_sessionID, 'no new session if unauthorized page is hit' ); + + # Luckily mainpage can still be accessed + $s->click( { id => 'mainpage', main_class => 'main container-fluid' } ); + like( $driver->get_title, qr(Koha staff interface), 'Patron can come back to the mainpage' ); + $cookie = $driver->get_cookie_named('CGISESSID'); + is( $cookie->{value}, $first_sessionID, 'no new session if back to the mainpage' ); + + # As well as the search + $driver->get( $s->base_url . q|catalogue/search.pl| ); + like( $driver->get_title, qr(Advanced search), 'Patron can access advanced search' ); + # But circulation module is prohibided! + $driver->get( $s->base_url . q|circ/circulation.pl| ); + like( $driver->get_title, qr(Access denied), 'Patron cannot access the circulation module' ); + # Still can reuse the same cookie + $cookie = $driver->get_cookie_named('CGISESSID'); + is( $cookie->{value}, $first_sessionID, 'no new session if unauthorized page is hit' ); + + # This is the "previous page" using the back() JS + $s->click( { id => 'previous_page', main_class => 'main container-fluid' } ); + like( $driver->get_title, qr(Advanced search), 'Patron can come back to the previous page' ); + $cookie = $driver->get_cookie_named('CGISESSID'); + is( $cookie->{value}, $first_sessionID, 'no new session if back to the previous page' ); + + # Check with a script that is using check_cookie_auth, session must not be deleted! + $driver->get( $s->base_url . q|svc/checkouts| ); + #FIXME - 500 is the current behaviour, but it's not nice. It could be improved. + like( $driver->get_title, qr(Error 500), 'Patron cannot access svc script where circulate permissions are required'); + $driver->get( $s->base_url . q|catalogue/search.pl| ); + like( $driver->get_title, qr(Advanced search), 'Patron can reuse the cookie after a script that used check_cookie_auth' ); + $cookie = $driver->get_cookie_named('CGISESSID'); + is( $cookie->{value}, $first_sessionID, 'no new session if unauthorized page is hit' ); + }; push @data_to_cleanup, $patron, $patron->category, $patron->library; }; subtest 'OPAC interface authentication' => sub { - plan tests => 6; + plan tests => 7; my $mainpage = $s->opac_base_url . q|opac-main.pl|; @@ -149,6 +212,67 @@ SKIP: { $driver->find_element('//a[@id="logout"]')->click; $driver->find_element('//div[@id="login"]'); # logged out + subtest 'not authorized' => sub { + plan tests => 13; + + $driver->get($mainpage . q|?logout.x=1|); + $driver->get($mainpage); + my $cookie = $driver->get_cookie_named('CGISESSID'); + my $first_sessionID = $cookie->{value}; + + # User is not logged in, navigation does not generate a new cookie + $driver->get( $s->opac_base_url . q|opac-search.pl| ); + like( $driver->get_title, qr(Advanced search) ); + $cookie = $driver->get_cookie_named('CGISESSID'); + is( $cookie->{value}, $first_sessionID, ); + + # Login + $driver->get($mainpage); + $s->fill_form( { userid => $patron->userid, password => $password } ); + $s->submit_form; + + # After logged in, the same cookie is reused + like( $driver->get_title, qr(Your library home) ); + $cookie = $driver->get_cookie_named('CGISESSID'); + is( $cookie->{value}, $first_sessionID, ); + $driver->get( $s->opac_base_url . q|opac-search.pl| ); + like( $driver->get_title, qr(Advanced search) ); + $cookie = $driver->get_cookie_named('CGISESSID'); + is( $cookie->{value}, $first_sessionID, ); + + # Logged in user can place holds + $driver->get( $s->opac_base_url . q|opac-reserve.pl| ); # We may need to pass a biblionumber here in the future + like( $driver->get_title, qr(Placing a hold) ); + $cookie = $driver->get_cookie_named('CGISESSID'); + is( $cookie->{value}, $first_sessionID, ); + + $driver->get($mainpage . q|?logout.x=1|); + + # FIXME This new get should not be needed, but the cookie is not modified right after logout + # However it's not the behavour when testing the UI + $driver->get($mainpage); + + # After logout a new cookie is generated, the previous session has been deleted + $cookie = $driver->get_cookie_named('CGISESSID'); + isnt( $cookie->{value}, $first_sessionID, ); + $first_sessionID = $cookie->{value}; + + $driver->get( $s->opac_base_url . q|svc/checkout_notes| ); + #FIXME - 500 is the current behaviour, but it's not nice. It could be improved. + like( $driver->get_title, qr(An error has occurred), 'Patron cannot access svc'); + # No new cookie generated + $cookie = $driver->get_cookie_named('CGISESSID'); + is( $cookie->{value}, $first_sessionID, ); + + $driver->get( $s->opac_base_url . q|opac-reserve.pl| ); + like( $driver->get_title, qr(Log in to your account) ); + + # Still no new cookie generated + $driver->get($mainpage); + $cookie = $driver->get_cookie_named('CGISESSID'); + is( $cookie->{value}, $first_sessionID, ); + }; + push @data_to_cleanup, $patron, $patron->category, $patron->library; }; From 7e591077d1fcd7c10351045476105458bc64face Mon Sep 17 00:00:00 2001 From: Martin Renvoize <martin.renvoize@ptfs-europe.com> Date: Wed, 16 Mar 2022 14:07:23 +0000 Subject: [PATCH 10/10] Bug 29915: (QA follow-up) Spelling Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> --- t/db_dependent/selenium/authentication.t | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/db_dependent/selenium/authentication.t b/t/db_dependent/selenium/authentication.t index bd86fa7796..25dc8ed324 100755 --- a/t/db_dependent/selenium/authentication.t +++ b/t/db_dependent/selenium/authentication.t @@ -249,7 +249,7 @@ SKIP: { $driver->get($mainpage . q|?logout.x=1|); # FIXME This new get should not be needed, but the cookie is not modified right after logout - # However it's not the behavour when testing the UI + # However it's not the behaviour when testing the UI $driver->get($mainpage); # After logout a new cookie is generated, the previous session has been deleted