Bugzilla – Attachment 123314 Details for
Bug 28785
Code in C4::Auth::checkauth is copy pasted
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 28785: Centralize cookie auth check in check_cookie_auth
Bug-28785-Centralize-cookie-auth-check-in-checkcoo.patch (text/plain), 17.82 KB, created by
Jonathan Druart
on 2021-07-30 12:37:47 UTC
(
hide
)
Description:
Bug 28785: Centralize cookie auth check in check_cookie_auth
Filename:
MIME Type:
Creator:
Jonathan Druart
Created:
2021-07-30 12:37:47 UTC
Size:
17.82 KB
patch
obsolete
>From 24ee8c683d2f844f540442147721af97fd3b2832 Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >Date: Thu, 29 Jul 2021 11:03:46 +0200 >Subject: [PATCH] Bug 28785: Centralize cookie auth check in check_cookie_auth > >This code is duplicated in 3 different places, we must call >check_cookie_auth instead. > >It makes check_cookie_auth returns a 'restricted' when >SessionRestrictionByIP is set and the IP changed. >It also returns a third parameters contained the old and new IP, to fill >the "info" hash in checkauth but apparently the oldip and newip >variables are not even used from the template. We may want to remove it >completely. > >No change is expected with this patch, the different authentication >methods should still work as before. > >Test plan: >Log in the staff and OPAC interfaces, logout. >Log in and call script that call the 3 different subroutines modified by >this patch. For instance you can list checkouts (that is using >check_cookie_auth) and display a patron's image (using check_api_auth). > >QA with good knowledge of the C4::Auth module and the different >authentication methods is required. >--- > C4/Auth.pm | 268 ++++++++++++++++++----------------------------------- > 1 file changed, 88 insertions(+), 180 deletions(-) > >diff --git a/C4/Auth.pm b/C4/Auth.pm >index fd327bf6187..eb47a5e4428 100644 >--- a/C4/Auth.pm >+++ b/C4/Auth.pm >@@ -868,109 +868,82 @@ sub checkauth { > elsif ( $emailaddress) { > # the Google OpenID Connect passes an email address > } >- elsif ( $sessionID = $query->cookie("CGISESSID") ) >- { # assignment, not comparison >- $session = get_session($sessionID); >- C4::Context->_new_userenv($sessionID); >- my ( $ip, $lasttime, $sessiontype ); >- my $s_userid = ''; >- if ($session) { >- $s_userid = $session->param('id') // ''; >- C4::Context->set_userenv( >- $session->param('number'), $s_userid, >- $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') >- ); >- Koha::Logger->get->debug(sprintf "AUTH_SESSION: (%s)\t%s %s - %s", map { $session->param($_) || q{} } qw(cardnumber firstname surname branch)); >- $ip = $session->param('ip'); >- $lasttime = $session->param('lasttime'); >- $userid = $s_userid; >- $sessiontype = $session->param('sessiontype') || ''; >- } >- if ( ( $query->param('koha_login_context') && ( $q_userid ne $s_userid ) ) >- || ( $cas && $query->param('ticket') && !C4::Context->userenv->{'id'} ) >- || ( $shib && $shib_login && !$logout && !C4::Context->userenv->{'id'} ) >- ) { >- >- #if a user enters an id ne to the id in the current session, we need to log them in... >- #first we need to clear the anonymous session... >- $anon_search_history = $session->param('search_history'); >- $session->delete(); >- $session->flush; >- C4::Context->_unset_userenv($sessionID); >- $sessionID = undef; >- $userid = undef; >- } >- elsif ($logout) { >+ elsif ( $sessionID = $query->cookie("CGISESSID") ) { # assignment, not comparison >+ my ( $return, $session, $more_info ) = check_cookie_auth( $sessionID, $flags, >+ { remote_addr => $ENV{REMOTE_ADDR} } >+ ); > >- # voluntary logout the user >- # check wether the user was using their shibboleth session or a local one >- my $shibSuccess = C4::Context->userenv->{'shibboleth'}; >- $session->delete(); >- $session->flush; >- C4::Context->_unset_userenv($sessionID); >+ if ( $return eq 'ok' ) { >+ Koha::Logger->get->debug(sprintf "AUTH_SESSION: (%s)\t%s %s - %s", map { $session->param($_) || q{} } qw(cardnumber firstname surname branch)); > >- $sessionID = undef; >- $userid = undef; >+ my $s_userid = $session->param('id'); >+ $userid = $s_userid; > >- if ($cas and $caslogout) { >- logout_cas($query, $type); >- } >+ if ( ( $query->param('koha_login_context') && ( $q_userid ne $s_userid ) ) >+ || ( $cas && $query->param('ticket') && !C4::Context->userenv->{'id'} ) >+ || ( $shib && $shib_login && !$logout && !C4::Context->userenv->{'id'} ) >+ ) { > >- # If we are in a shibboleth session (shibboleth is enabled, a shibboleth match attribute is set and matches koha matchpoint) >- if ( $shib and $shib_login and $shibSuccess) { >- logout_shib($query); >+ #if a user enters an id ne to the id in the current session, we need to log them in... >+ #first we need to clear the anonymous session... >+ $anon_search_history = $session->param('search_history'); >+ $session->delete(); >+ $session->flush; >+ C4::Context->_unset_userenv($sessionID); > } >- } >- elsif ( !$lasttime || ( $lasttime < time() - $timeout ) ) { >+ elsif ($logout) { > >- # timed logout >- $info{'timed_out'} = 1; >- if ($session) { >+ # voluntary logout the user >+ # check wether the user was using their shibboleth session or a local one >+ my $shibSuccess = C4::Context->userenv->{'shibboleth'}; > $session->delete(); > $session->flush; >- } >- C4::Context->_unset_userenv($sessionID); >+ C4::Context->_unset_userenv($sessionID); > >- $userid = undef; >- $sessionID = undef; >- } >- elsif ( C4::Context->preference('SessionRestrictionByIP') && $ip ne $ENV{'REMOTE_ADDR'} ) { >+ if ($cas and $caslogout) { >+ logout_cas($query, $type); >+ } > >- # Different ip than originally logged in from >- $info{'oldip'} = $ip; >- $info{'newip'} = $ENV{'REMOTE_ADDR'}; >- $info{'different_ip'} = 1; >- $session->delete(); >- $session->flush; >- C4::Context->_unset_userenv($sessionID); >+ # If we are in a shibboleth session (shibboleth is enabled, a shibboleth match attribute is set and matches koha matchpoint) >+ if ( $shib and $shib_login and $shibSuccess) { >+ logout_shib($query); >+ } >+ } else { > >- $sessionID = undef; >- $userid = undef; >- } >- else { >- $cookie = $query->cookie( >- -name => 'CGISESSID', >- -value => $session->id, >- -HttpOnly => 1, >- -secure => ( C4::Context->https_enabled() ? 1 : 0 ), >- ); >- $session->param( 'lasttime', time() ); >- 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; >+ $cookie = $query->cookie( >+ -name => 'CGISESSID', >+ -value => $session->id, >+ -HttpOnly => 1, >+ -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; >+ } > } > } >+ } elsif ( !$logout ) { >+ if ( $return eq 'expired' ) { >+ $info{timed_out} = 1; >+ } elsif ( $return eq 'restricted' ) { >+ $info{oldip} = $more_info->{old_ip}; >+ $info{newip} = $more_info->{new_ip}; >+ $info{different_ip} = 1; >+ } > } > } >- unless ( $userid || $sessionID ) { >+ >+ 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()"; > >@@ -985,7 +958,7 @@ sub checkauth { > C4::Context->_new_userenv($sessionID); > $cookie = $query->cookie( > -name => 'CGISESSID', >- -value => $session->id, >+ -value => $sessionID, > -HttpOnly => 1, > -secure => ( C4::Context->https_enabled() ? 1 : 0 ), > ); >@@ -1317,8 +1290,8 @@ sub checkauth { > script_name => get_script_name(), > casAuthentication => C4::Context->preference("casAuthentication"), > shibbolethAuthentication => $shib, >- SessionRestrictionByIP => C4::Context->preference("SessionRestrictionByIP"), > suggestion => C4::Context->preference("suggestion"), >+ SessionRestrictionByIP => C4::Context->preference("SessionRestrictionByIP"), > virtualshelves => C4::Context->preference("virtualshelves"), > LibraryName => "" . C4::Context->preference("LibraryName"), > LibraryNameTitle => "" . $LibraryNameTitle, >@@ -1486,80 +1459,27 @@ sub check_api_auth { > return ( "maintenance", undef, undef ); > } > >- # FIXME -- most of what follows is a copy-and-paste >- # of code from checkauth. There is an obvious need >- # for refactoring to separate the various parts of >- # the authentication code, but as of 2007-11-19 this >- # is deferred so as to not introduce bugs into the >- # regular authentication code for Koha 3.0. >- >- # see if we have a valid session cookie already >- # however, if a userid parameter is present (i.e., from >- # a form submission, assume that any current cookie >- # is to be ignored >- my $sessionID = undef; >+ my ( $sessionID, $session ); > unless ( $query->param('userid') ) { > $sessionID = $query->cookie("CGISESSID"); > } > if ( $sessionID && not( $cas && $query->param('PT') ) ) { >- 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 $ip = $session->param('ip'); >- my $lasttime = $session->param('lasttime'); >- my $userid = $session->param('id'); >- if ( $lasttime < time() - $timeout ) { >+ my $return; >+ ( $return, $session, undef ) = check_cookie_auth( >+ $sessionID, $flagsrequired, { remote_addr => $ENV{REMOTE_ADDR} } ); > >- # time out >- $session->delete(); >- $session->flush; >- C4::Context->_unset_userenv($sessionID); >- $userid = undef; >- $sessionID = undef; >- return ( "expired", undef, undef ); >- } elsif ( C4::Context->preference('SessionRestrictionByIP') && $ip ne $ENV{'REMOTE_ADDR'} ) { >+ return ( $return, undef, undef ) # Cookie auth failed >+ if $return ne "ok"; >+ >+ my $cookie = $query->cookie( >+ -name => 'CGISESSID', >+ -value => $session->id, >+ -HttpOnly => 1, >+ -secure => ( C4::Context->https_enabled() ? 1 : 0 ), >+ ); >+ return ( $return, undef, $session ); > >- # IP address changed >- $session->delete(); >- $session->flush; >- C4::Context->_unset_userenv($sessionID); >- $userid = undef; >- $sessionID = undef; >- return ( "expired", undef, undef ); >- } else { >- my $cookie = $query->cookie( >- -name => 'CGISESSID', >- -value => $session->id, >- -HttpOnly => 1, >- -secure => ( C4::Context->https_enabled() ? 1 : 0 ), >- ); >- $session->param( 'lasttime', time() ); >- my $flags = haspermission( $userid, $flagsrequired ); >- if ($flags) { >- return ( "ok", $cookie, $sessionID ); >- } else { >- $session->delete(); >- $session->flush; >- C4::Context->_unset_userenv($sessionID); >- $userid = undef; >- $sessionID = undef; >- return ( "failed", undef, undef ); >- } >- } >- } else { >- return ( "expired", undef, undef ); >- } > } else { > > # new login >@@ -1712,18 +1632,18 @@ Possible return values in C<$status> are: > > =item "expired -- session cookie has expired; API user should resubmit userid and password > >+=item "restricted" -- The IP has changed (if SessionRestrictionByIP) >+ > =back > > =cut > > sub check_cookie_auth { >- my $cookie = shift; >+ my $sessionID = shift; > my $flagsrequired = shift; > my $params = shift; > > my $remote_addr = $params->{remote_addr} || $ENV{REMOTE_ADDR}; >- my $dbh = C4::Context->dbh; >- my $timeout = _timeout_syspref(); > > unless ( C4::Context->preference('Version') ) { > >@@ -1740,27 +1660,19 @@ sub check_cookie_auth { > return ( "maintenance", undef ); > } > >- # FIXME -- most of what follows is a copy-and-paste >- # of code from checkauth. There is an obvious need >- # for refactoring to separate the various parts of >- # the authentication code, but as of 2007-11-23 this >- # is deferred so as to not introduce bugs into the >- # regular authentication code for Koha 3.0. >- > # see if we have a valid session cookie already > # however, if a userid parameter is present (i.e., from > # a form submission, assume that any current cookie > # is to be ignored >- unless ( defined $cookie and $cookie ) { >+ unless ( defined $sessionID and $sessionID ) { > return ( "failed", undef ); > } >- my $sessionID = $cookie; > 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('number'), $session->param('id') // '', > $session->param('cardnumber'), $session->param('firstname'), > $session->param('surname'), $session->param('branch'), > $session->param('branchname'), $session->param('flags'), >@@ -1769,17 +1681,17 @@ sub check_cookie_auth { > $session->param('register_id'), $session->param('register_name') > ); > >+ my $userid = $session->param('id'); > my $ip = $session->param('ip'); > my $lasttime = $session->param('lasttime'); >- my $userid = $session->param('id'); >- if ( $lasttime < time() - $timeout ) { >+ my $timeout = _timeout_syspref(); >+ >+ if ( !$lasttime || ( $lasttime < time() - $timeout ) ) { > > # time out > $session->delete(); > $session->flush; > C4::Context->_unset_userenv($sessionID); >- $userid = undef; >- $sessionID = undef; > return ("expired", undef); > } elsif ( C4::Context->preference('SessionRestrictionByIP') && $ip ne $remote_addr ) { > >@@ -1787,20 +1699,16 @@ sub check_cookie_auth { > $session->delete(); > $session->flush; > C4::Context->_unset_userenv($sessionID); >- $userid = undef; >- $sessionID = undef; >- return ( "expired", undef ); >+ return ( "restricted", undef, { old_ip => $ip, new_ip => $remote_addr}); > } else { > $session->param( 'lasttime', time() ); > my $flags = defined($flagsrequired) ? haspermission( $userid, $flagsrequired ) : 1; > if ($flags) { >- return ( "ok", $sessionID ); >+ return ( "ok", $session ); > } else { > $session->delete(); > $session->flush; > C4::Context->_unset_userenv($sessionID); >- $userid = undef; >- $sessionID = undef; > return ( "failed", undef ); > } > } >-- >2.20.1
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 28785
:
123314
|
123315
|
123316
|
123317
|
123319
|
123320
|
123321
|
123322
|
123323
|
123392
|
123393
|
123394
|
123395
|
124776
|
124777
|
124778
|
124779