From 58083ddf9317f577fbdc7384d37f442a08819679 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Mon, 11 Mar 2024 14:30:25 +0100 Subject: [PATCH] Bug 36367: Remove _new_userenv TODO - better review C4::Auth's changes. Are all the removal of _new_userenv correct/enough? Signed-off-by: Julian Maurice Signed-off-by: Martin Renvoize --- C4/Auth.pm | 14 ++---- C4/Context.pm | 48 +++---------------- C4/InstallAuth.pm | 6 +-- Koha/BackgroundJob.pm | 1 - Koha/Middleware/UserEnv.pm | 2 +- Koha/REST/V1/Auth.pm | 2 +- Koha/Script.pm | 2 - opac/ilsdi.pl | 1 - t/Token.t | 3 -- t/db_dependent/Auth.t | 7 +-- t/db_dependent/Koha/BackgroundJob.t | 1 - t/db_dependent/Koha/Items.t | 2 +- t/db_dependent/Koha/Reviews.t | 1 - .../Koha/Template/Plugin/Registers.t | 4 +- t/db_dependent/Patron/Borrower_Discharge.t | 1 - t/lib/Mocks.pm | 2 - 16 files changed, 20 insertions(+), 77 deletions(-) diff --git a/C4/Auth.pm b/C4/Auth.pm index 17bdef4465..6e75267c7c 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -933,7 +933,7 @@ sub checkauth { $session->delete(); $session->flush; $cookie = $cookie_mgr->clear_unless( $query->cookie, @$cookie ); - C4::Context::_unset_userenv($sessionID); + C4::Context::unset_userenv(); $sessionID = undef; undef $userid; # IMPORTANT: this assures us a new session in code below $auth_state = 'failed'; @@ -982,7 +982,7 @@ sub checkauth { $session->delete(); $session->flush; } - C4::Context::_unset_userenv($sessionID); + C4::Context::unset_userenv(); $cookie = $cookie_mgr->clear_unless( $query->cookie, @$cookie ); if ($cas and $caslogout) { @@ -1012,7 +1012,6 @@ sub checkauth { } $sessionID = $session->id; - C4::Context->_new_userenv($sessionID); $cookie = $cookie_mgr->replace_in_list( $cookie, $query->cookie( -name => 'CGISESSID', -value => $sessionID, @@ -1158,7 +1157,7 @@ sub checkauth { # although we do present an authorization failure. (Yes, the # authentication was actually correct.) $info{'nopermission'} = 1; - C4::Context::_unset_userenv($sessionID); + C4::Context::unset_userenv(); } my ( $borrowernumber, $firstname, $surname, $userflags, $branchcode, $branchname, $emailaddress, $desk_id, @@ -1306,7 +1305,7 @@ sub checkauth { else { if ($userid) { $info{'invalid_username_or_password'} = 1; - C4::Context::_unset_userenv($sessionID); + C4::Context::unset_userenv(); } $session->param( 'lasttime', time() ); $session->param( 'ip', $session->remote_addr() ); @@ -1667,7 +1666,6 @@ sub check_api_auth { return ( "failed", undef, undef ) unless $session; my $sessionID = $session->id; - C4::Context->_new_userenv($sessionID); my $cookie = $query->cookie( -name => 'CGISESSID', -value => $sessionID, @@ -1831,7 +1829,7 @@ sub check_cookie_auth { unless ( $sessionID ) { return ( "failed", undef ); } - C4::Context::_unset_userenv($sessionID); # remove old userenv first + C4::Context::unset_userenv(); my $session = get_session($sessionID); if ($session) { my $userid = $session->param('id'); @@ -1868,7 +1866,6 @@ sub check_cookie_auth { return ("password_expired", undef ) if $patron->password_expired; my $flags = defined($flagsrequired) ? haspermission( $userid, $flagsrequired ) : 1; if ($flags) { - C4::Context->_new_userenv($sessionID); if ( !C4::Context->interface ) { # No need to override the interface, most often set by get_template_and_user C4::Context->interface( $session->param('interface') ); @@ -1898,7 +1895,6 @@ sub check_cookie_auth { } } else { - C4::Context->_new_userenv($sessionID); C4::Context->interface($session->param('interface')); C4::Context->set_userenv( undef, q{} ); return ( "anon", $session ); diff --git a/C4/Context.pm b/C4/Context.pm index 85191371d5..8c101c01b7 100644 --- a/C4/Context.pm +++ b/C4/Context.pm @@ -182,7 +182,6 @@ sub new { $self->{"Zconn"} = undef; # Zebra Connections $self->{"userenv"} = undef; # User env - $self->{"activeuser"} = undef; # current active user $self->{"shelves"} = undef; $self->{tz} = undef; # local timezone object @@ -758,19 +757,10 @@ sub restore_dbh Retrieves a hash for user environment variables. -This hash shall be cached for future use: if you call -Cuserenv> twice, you will get the same hash without real DB access - =cut -#' sub userenv { - my $var = $context->{"activeuser"}; - if (defined $var and defined $context->{"userenv"}->{$var}) { - return $context->{"userenv"}->{$var}; - } else { - return; - } + return $context->{userenv}; } =head2 set_userenv @@ -798,7 +788,6 @@ sub set_userenv { $register_id, $register_name ) = @_; - my $var=$context->{"activeuser"} || ''; my $cell = { "number" => $usernum, "id" => $userid, @@ -817,44 +806,21 @@ sub set_userenv { "register_id" => $register_id, "register_name" => $register_name }; - $context->{userenv}->{$var} = $cell; + $context->{userenv} = $cell; return $cell; } -=head2 _new_userenv - - C4::Context->_new_userenv($session); # FIXME: This calling style is wrong for what looks like an _internal function - -Builds a hash for user environment variables. - -This hash shall be cached for future use: if you call -Cuserenv> twice, you will get the same hash without real DB access - -_new_userenv is called in Auth.pm - -=cut - -#' -sub _new_userenv -{ - shift; # Useless except it compensates for bad calling style - my ($sessionID)= @_; - $context->{"activeuser"}=$sessionID; -} - -=head2 _unset_userenv +=head2 unset_userenv - C4::Context->_unset_userenv; + C4::Context->unset_userenv; -Destroys the hash for activeuser user environment variables. +Destroys user environment variables. =cut -#' - -sub _unset_userenv +sub unset_userenv { - delete $context->{activeuser}; + $context->{userenv} = {}; } diff --git a/C4/InstallAuth.pm b/C4/InstallAuth.pm index 7dc0284e32..fa52cbcfc5 100644 --- a/C4/InstallAuth.pm +++ b/C4/InstallAuth.pm @@ -252,7 +252,6 @@ sub checkauth { my $session = Koha::Session->get_session( { sessionID => $sessionID, storage_method => 'file' } ); if ( $session ) { - C4::Context->_new_userenv($sessionID); if ( $session->param('cardnumber') ) { C4::Context->set_userenv( $session->param('number'), @@ -279,7 +278,7 @@ sub checkauth { if ($logout || !$session) { # voluntary logout the user - C4::Context->_unset_userenv($sessionID); + C4::Context->unset_userenv(); $session = Koha::Session->get_session( { storage_method => 'file' } ); } @@ -288,7 +287,6 @@ sub checkauth { unless ($userid) { $userid = $query->param('login_userid'); my $password = $query->param('login_password'); - C4::Context->_new_userenv($sessionID); my ( $return, $cardnumber ) = checkpw( $userid, $password ); if ($return) { $loggedin = 1; @@ -334,7 +332,7 @@ sub checkauth { else { if ($userid) { $info{'invalid_username_or_password'} = 1; - C4::Context->_unset_userenv($sessionID); + C4::Context->unset_userenv(); } } } diff --git a/Koha/BackgroundJob.pm b/Koha/BackgroundJob.pm index 704843549c..4525122e8c 100644 --- a/Koha/BackgroundJob.pm +++ b/Koha/BackgroundJob.pm @@ -170,7 +170,6 @@ sub process { if ( $self->context ) { my $context = $self->json->decode($self->context); - C4::Context->_new_userenv(-1); C4::Context->interface( $context->{interface} ); C4::Context->set_userenv( $context->{number}, $context->{id}, diff --git a/Koha/Middleware/UserEnv.pm b/Koha/Middleware/UserEnv.pm index ab67fab913..14aef47b79 100644 --- a/Koha/Middleware/UserEnv.pm +++ b/Koha/Middleware/UserEnv.pm @@ -22,7 +22,7 @@ sub call { my $req = Plack::Request->new($env); - C4::Context->_unset_userenv; + C4::Context->unset_userenv; return $self->app->($env); } diff --git a/Koha/REST/V1/Auth.pm b/Koha/REST/V1/Auth.pm index e563315418..5f585b27de 100644 --- a/Koha/REST/V1/Auth.pm +++ b/Koha/REST/V1/Auth.pm @@ -568,7 +568,7 @@ sub _set_userenv { $THE_library = $patron->library; } - C4::Context->_new_userenv( $patron->borrowernumber ); + C4::Context->new_userenv(); C4::Context->set_userenv( $patron->borrowernumber, # number, $patron->userid, # userid, diff --git a/Koha/Script.pm b/Koha/Script.pm index f41a45cad7..1db7ad96f6 100644 --- a/Koha/Script.pm +++ b/Koha/Script.pm @@ -46,11 +46,9 @@ sub import { my $class = shift; my @flags = @_; - C4::Context->_new_userenv(1); if ( ( $flags[0] || '' ) eq '-cron' ) { # Set userenv - C4::Context->_new_userenv(1); C4::Context->set_userenv( undef, undef, undef, 'CRON', 'CRON', undef, undef, undef, undef, undef diff --git a/opac/ilsdi.pl b/opac/ilsdi.pl index 5b105246fa..4cf4818aa4 100755 --- a/opac/ilsdi.pl +++ b/opac/ilsdi.pl @@ -128,7 +128,6 @@ unless ( $cgi->param('service') ) { } # Set the userenv -C4::Context->_new_userenv( 'ILSDI_'.time() ); C4::Context->set_userenv( undef, undef, undef, 'ILSDI', 'ILSDI', undef, undef, undef, undef, undef, diff --git a/t/Token.t b/t/Token.t index 355282c6ed..422feb21c9 100755 --- a/t/Token.t +++ b/t/Token.t @@ -27,7 +27,6 @@ use Time::HiRes qw|usleep|; use C4::Context; use Koha::Token; -C4::Context->_new_userenv('DUMMY SESSION'); C4::Context->set_userenv(0,42,0,'firstname','surname', 'CPL', 'Library 1', 0, ''); my $tokenizer = Koha::Token->new; @@ -114,14 +113,12 @@ subtest 'testing _add_default_csrf_params with/without userenv (bug 27849)' => s is( $result->{id}, 'anonymous_567', 'Check userid' ); # Clear userenv - C4::Context::_unset_userenv('DUMMY SESSION'); is( C4::Context::userenv, undef, 'No userenv anymore' ); $result = Koha::Token::_add_default_csrf_params({}); # pass no session_id is( $result->{session_id}, Koha::Token::DEFA_SESSION_ID, 'Check session id' ); is( $result->{id}, Koha::Token::DEFA_SESSION_USERID. '_'. $result->{session_id}, 'Check userid' ); # Empty anonymous userenv (see C4::Auth::check_cookie_auth) - C4::Context->_new_userenv('ANON SESSION'); C4::Context->set_userenv( undef, q{} ); ok( C4::Context->userenv, "Userenv exists" ); $result = Koha::Token::_add_default_csrf_params( {} ); # pass no session_id diff --git a/t/db_dependent/Auth.t b/t/db_dependent/Auth.t index ce7c7cffa9..101102aea0 100755 --- a/t/db_dependent/Auth.t +++ b/t/db_dependent/Auth.t @@ -199,7 +199,6 @@ subtest 'checkauth() tests' => sub { $session->param( 'interface', 'intranet' ); $session->flush; my $sessionID = $session->id; - C4::Context->_new_userenv($sessionID); my ($return) = C4::Auth::check_cookie_auth( $sessionID, undef, { skip_version_check => 1, remote_addr => '1.2.3.4' } ); @@ -233,7 +232,6 @@ subtest 'checkauth() tests' => sub { $session->param( 'interface', 'opac' ); $session->flush; my $previous_sessionID = $session->id; - C4::Context->_new_userenv($previous_sessionID); my ( $return ) = C4::Auth::check_cookie_auth( $previous_sessionID, undef, { skip_version_check => 1, remote_addr => '1.2.3.4' } ); is( $return, 'ok', 'Former session in shape now' ); @@ -262,7 +260,6 @@ subtest 'checkauth() tests' => sub { $session->param( 'interface', 'intranet' ); $session->flush; $previous_sessionID = $session->id; - C4::Context->_new_userenv($previous_sessionID); $cgi->param( -name => 'login_userid', -value => $patron2->userid ); $cgi->param( -name => 'login_password', -value => $password ); $cgi->param( -name => 'koha_login_context', -value => 1 ); @@ -417,7 +414,6 @@ subtest 'checkauth() tests' => sub { is( $sesh->param('branch'), $branch->branchcode, "If user is superlibrarian, they should be able to choose a branch" ); }; - C4::Context->_new_userenv; # For next tests }; subtest 'no_set_userenv parameter tests' => sub { @@ -430,8 +426,8 @@ subtest 'no_set_userenv parameter tests' => sub { my $password = set_weak_password($patron); ok( checkpw( $patron->userid, $password, undef, undef, 1 ), 'checkpw returns true' ); + C4::Context->unset_userenv; is( C4::Context->userenv, undef, 'Userenv should be undef as required' ); - C4::Context->_new_userenv('DUMMY SESSION'); C4::Context->set_userenv(0,0,0,'firstname','surname', $library->branchcode, 'Library 1', 0, '', ''); is( C4::Context->userenv->{branch}, $library->branchcode, 'Userenv gives correct branch' ); ok( checkpw( $patron->userid, $password, undef, undef, 1 ), 'checkpw returns true' ); @@ -940,7 +936,6 @@ subtest 'Userenv clearing in check_cookie_auth' => sub { 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 ); diff --git a/t/db_dependent/Koha/BackgroundJob.t b/t/db_dependent/Koha/BackgroundJob.t index 5db21a0b34..8511551adc 100755 --- a/t/db_dependent/Koha/BackgroundJob.t +++ b/t/db_dependent/Koha/BackgroundJob.t @@ -229,7 +229,6 @@ subtest 'process tests' => sub { { size => 10, a => 'aaa', b => 'bbb' } ); my $job = Koha::BackgroundJobs->find($job_id); - C4::Context->_new_userenv(-1); C4::Context->interface('opac'); is( C4::Context->userenv, undef, "Userenv unset prior to calling process"); is( C4::Context->interface, 'opac', "Interface set to opac prior to calling process"); diff --git a/t/db_dependent/Koha/Items.t b/t/db_dependent/Koha/Items.t index ddf47a871a..3a0ffd11cc 100755 --- a/t/db_dependent/Koha/Items.t +++ b/t/db_dependent/Koha/Items.t @@ -1317,7 +1317,7 @@ subtest 'store' => sub { C4::Circulation::LostItem( $item->itemnumber, 1 ); # Unset the userenv - C4::Context->_new_userenv(undef); + C4::Context->unset_userenv(); # Simluate item marked as found $item->itemlost(0)->store; diff --git a/t/db_dependent/Koha/Reviews.t b/t/db_dependent/Koha/Reviews.t index 7e947a1bb8..57c651f43c 100755 --- a/t/db_dependent/Koha/Reviews.t +++ b/t/db_dependent/Koha/Reviews.t @@ -70,7 +70,6 @@ is( $retrieved_review_1_1->review, $new_review_1_1->review, 'Find a review by id subtest 'search_limited' => sub { plan tests => 2; - C4::Context->_new_userenv('xxx'); my $group_1 = Koha::Library::Group->new( { title => 'TEST Group 1' } )->store; my $group_2 = Koha::Library::Group->new( { title => 'TEST Group 2' } )->store; Koha::Library::Group->new({ parent_id => $group_1->id, branchcode => $patron_1->branchcode })->store(); diff --git a/t/db_dependent/Koha/Template/Plugin/Registers.t b/t/db_dependent/Koha/Template/Plugin/Registers.t index 3e110ff6c5..9c361bde70 100755 --- a/t/db_dependent/Koha/Template/Plugin/Registers.t +++ b/t/db_dependent/Koha/Template/Plugin/Registers.t @@ -45,7 +45,7 @@ subtest 'session_register_id' => sub { '1', "Returns the register id when set in the userenv" ); # Unset the userenv - C4::Context->_new_userenv(undef); + C4::Context->unset_userenv(); }; subtest 'session_register_name' => sub { @@ -61,7 +61,7 @@ subtest 'session_register_name' => sub { 'Register One', "Returns the register name when set in the userenv" ); # Unset the userenv - C4::Context->_new_userenv(undef); + C4::Context->unset_userenv(); }; subtest 'all() tests' => sub { diff --git a/t/db_dependent/Patron/Borrower_Discharge.t b/t/db_dependent/Patron/Borrower_Discharge.t index 062a61076e..6c06ecff90 100755 --- a/t/db_dependent/Patron/Borrower_Discharge.t +++ b/t/db_dependent/Patron/Borrower_Discharge.t @@ -47,7 +47,6 @@ my $library = $builder->build({ source => 'Branch' }); my $another_library = $builder->build({ source => 'Branch' }); my $itemtype = $builder->build({ source => 'Itemtype' })->{itemtype}; -C4::Context->_new_userenv('xxx'); my $patron = $builder->build_object({ class => 'Koha::Patrons', value => { diff --git a/t/lib/Mocks.pm b/t/lib/Mocks.pm index bca34f4e0b..843ee9bc53 100644 --- a/t/lib/Mocks.pm +++ b/t/lib/Mocks.pm @@ -119,8 +119,6 @@ Also, some sane defaults are set if no parameters are passed. sub mock_userenv { my ( $params ) = @_; - C4::Context->_new_userenv(42); - my $userenv; if ( $params and my $patron = $params->{patron} ) { $userenv = $patron->unblessed; -- 2.39.5