Attachment 167382 Details for Bug 36367 – Bug 36367: Remove _new_userenv

[patch] Bug 36367: Remove _new_userenv

Bug-36367-Remove-newuserenv.patch (text/plain), 18.01 KB, created by Martin Renvoize (ashimema) on 2024-06-04 12:10:52 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Martin Renvoize (ashimema)

Created: 2024-06-04 12:10:52 UTC

Size: 18.01 KB

patch

obsolete

>From ba30e0aabbd5ae864fe06295fd39d3db44ce12a1 Mon Sep 17 00:00:00 2001
>From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
>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 <julian.maurice@biblibre.com>
>Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>---
> 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 bc1f2c945e2..c8d26bbe40f 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 85191371d50..8c101c01b71 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
>-C<C4::Context-E<gt>userenv> 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
>-C<C4::Context-E<gt>userenv> 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 704843549c9..4525122e8c3 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 ab67fab913a..14aef47b799 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 e5633154184..5f585b27def 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 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 ce7c7cffa9c..101102aea0f 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 ddf47a871aa..3a0ffd11cc0 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.45.1

Actions: View | Diff | Splinter Review

Attachments on bug 36367: 163500 | 163501 | 163502 | 163503 | 163504 | 163505 | 163506 | 163507 | 163508 | 165115 | 165116 | 165117 | 165118 | 165119 | 165120 | 165121 | 165122 | 165123 | 165124 | 165125 | 165126 | 167380 | 167381 | 167382 | 167383 | 167384 | 167385 | 167386 | 167387 | 167388 | 167389 | 167390 | 167391