From eeb899995468b6bef8ea0bad150e4613699263d6 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Mon, 11 Mar 2024 14:30:25 +0100 Subject: [PATCH] Bug 36149: Remove _new_userenv TODO - better review C4::Auth's changes. Are all the removal of _new_userenv correct/enough? --- 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 a8c21740f00..a5bb0c39648 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -925,7 +925,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'; @@ -974,7 +974,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) { @@ -1004,7 +1004,6 @@ sub checkauth { } $sessionID = $session->id; - C4::Context->_new_userenv($sessionID); $cookie = $cookie_mgr->replace_in_list( $cookie, $query->cookie( -name => 'CGISESSID', -value => $sessionID, @@ -1150,7 +1149,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, @@ -1281,7 +1280,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() ); @@ -1642,7 +1641,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, @@ -1806,7 +1804,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'); @@ -1843,7 +1841,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') ); @@ -1873,7 +1870,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 00afbb38a0b..2b022c69342 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 @@ -757,19 +756,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 @@ -797,7 +787,6 @@ sub set_userenv { $register_id, $register_name ) = @_; - my $var=$context->{"activeuser"} || ''; my $cell = { "number" => $usernum, "id" => $userid, @@ -816,44 +805,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 7dc0284e324..fa52cbcfc56 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 579b0f93272..f848a21af10 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 03d515fdcff..694a75b7b42 100644 --- a/Koha/Middleware/UserEnv.pm +++ b/Koha/Middleware/UserEnv.pm @@ -10,7 +10,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 948bed689bd..bf609bc9862 100644 --- a/Koha/REST/V1/Auth.pm +++ b/Koha/REST/V1/Auth.pm @@ -569,7 +569,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 f41a45cad7f..1db7ad96f62 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 5b105246fa4..4cf4818aa40 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 355282c6ed8..422feb21c9a 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 99381c9365d..2e7aa7c0044 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 5db21a0b34a..8511551adcd 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 26f81dd7d4a..ca901393ea9 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 7e947a1bb86..57c651f43c1 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 3e110ff6c5b..9c361bde70b 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 062a61076e0..6c06ecff90e 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 bca34f4e0b2..843ee9bc53e 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.34.1