Merge remote-tracking branch 'security/21.11.x' into 21.11.x

This commit is contained in:
Kyle M Hall 2022-03-24 14:19:19 -04:00
commit 5f4ee6fd6e
5 changed files with 358 additions and 52 deletions

View File

@ -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;
@ -560,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;
}
}
@ -877,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 }
);
@ -897,7 +900,8 @@ sub checkauth {
$anon_search_history = $session->param('search_history');
$session->delete();
$session->flush;
C4::Context->_unset_userenv($sessionID);
C4::Context::_unset_userenv($sessionID);
$sessionID = undef;
}
elsif ($logout) {
@ -906,7 +910,8 @@ sub checkauth {
my $shibSuccess = C4::Context->userenv->{'shibboleth'};
$session->delete();
$session->flush;
C4::Context->_unset_userenv($sessionID);
C4::Context::_unset_userenv($sessionID);
$sessionID = undef;
if ($cas and $caslogout) {
logout_cas($query, $type);
@ -925,14 +930,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 ) {
@ -947,13 +949,12 @@ sub checkauth {
}
unless ( $loggedin ) {
$sessionID = undef;
$userid = undef;
}
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
@ -1102,7 +1103,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,
@ -1227,7 +1228,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() );
@ -1245,6 +1246,7 @@ sub checkauth {
$session->param( 'sessiontype', 'anon' );
$session->param( 'interface', $type);
}
$session->flush;
} # END unless ($userid)
# finished authentification, now respond
@ -1622,7 +1624,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>
@ -1685,57 +1687,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 );
}
@ -1781,9 +1783,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;
}

View File

@ -218,6 +218,7 @@ sub set_to_session {
return () unless $session;
$session->param( 'search_history',
uri_escape( encode_json($search_history) ) );
$session->flush;
}
1;

View File

@ -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 %]

View File

@ -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 => 15;
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,176 @@ 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 => 31;
# 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{<title>\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
{
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);
# Sending undefined fails obviously
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( $first_sessionID, {catalogue => 1} );
is( $auth_status, "expired");
is( $session, undef );
{
# Trying to access without sessionID
$cgi = CGI->new;
( $auth_status, $session) = C4::Auth::check_cookie_auth(undef, {catalogue => 1});
is( $auth_status, 'failed' );
is( $session, undef );
# 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 );
}
{
# First logging in
$cgi = CGI->new;
$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;
# Patron does not have the borrowers permission
# $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 );
# 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' );
# 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});
is( $userid, $patron->userid );
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->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;

View File

@ -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 behaviour 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;
};