From 49664b00bb1f02c969215f6555ea18734c730e23 Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Tue, 21 May 2024 12:44:25 +0000 Subject: [PATCH] Bug 36908: Sort branches based on branchcode This adds a sort based on branchcode, it's a fallback for an edge case that should be rare so I think is acceptable, as long as documented. I added test coverage, but it may no longer be possible to encounter this scenario. System preference descriptions are updated as well. Signed-off-by: Jonathan Druart Signed-off-by: Martin Renvoize Signed-off-by: Katrin Fischer (cherry picked from commit 46cecfdd72ee45bfad03c6875fdc078df48eabec) Signed-off-by: Fridolin Somers (cherry picked from commit 7682763fca0e1d14c27a32e9987e8c66712229cb) Signed-off-by: Lucas Gass --- C4/Auth.pm | 5 ++- .../en/modules/admin/preferences/admin.pref | 6 +-- t/db_dependent/Auth.t | 44 +++++++++++++++---- 3 files changed, 41 insertions(+), 14 deletions(-) diff --git a/C4/Auth.pm b/C4/Auth.pm index 0f3516b600..c628f2f245 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -1195,8 +1195,8 @@ sub checkauth { $register_id = $register->id if ($register); $register_name = $register->name if ($register); } - my $branches = { map { $_->branchcode => $_->unblessed } Koha::Libraries->search->as_list }; if ( $type ne 'opac' ) { + my $branches = { map { $_->branchcode => $_->unblessed } Koha::Libraries->search->as_list }; if ( C4::Context->preference('AutoLocation') ) { # we have to check they are coming from the right ip range my $domain = $branches->{$branchcode}->{'branchip'}; @@ -1229,7 +1229,8 @@ sub checkauth { && $branches->{$branchcode}->{'branchip'} ) ) { - foreach my $br ( uniq( $branchcode, keys %$branches ) ) { + my @branchcodes = sort { lc $a cmp lc $b } keys %$branches; + foreach my $br ( uniq( $branchcode, @branchcodes ) ) { # now we work with the treatment of ip my $domain = $branches->{$br}->{'branchip'}; diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref index 447f4f95d3..cedf157689 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref @@ -91,14 +91,14 @@ Administration: class: integer - Adding d will specify it in days, e.g. 1d is timeout of one day. - - - "Require staff to log in from a computer in the IP address range specified by their library (if any): " + - "Require staff to log in from a computer in the IP address range specified by their library or the branch chosen at login (if any): " - pref: AutoLocation default: 0 choices: 1: "Yes" 0: "No" - Link to library administration - - 'This setting will override the StaffLoginBranchBasedOnIP system preference.' + - 'Staff can only choose thier branch on login if they have "loggedinlibrary" permission. This setting will override the StaffLoginBranchBasedOnIP system preference. In the event of multiple branches with matching IPs, branchcode (alphabetically) will be the tie breaker.' - - "Enable check for change in remote IP address for session security: " - pref: SessionRestrictionByIP @@ -115,7 +115,7 @@ Administration: 1: "Yes" 0: "No" - "Note: If IPs overlap, the first found match will be used." - - 'This setting will be overridden by AutoLocation system preference.' + - 'This setting will be overridden by AutoLocation system preference. In the event of multiple branches with matching IPs, branchcode (alphabetically) will be the tie breaker.' # PostgreSQL is supported by CGI::Session but not by Koha. - - "Storage of login session information: " diff --git a/t/db_dependent/Auth.t b/t/db_dependent/Auth.t index 787bb39b4c..b342b314bf 100755 --- a/t/db_dependent/Auth.t +++ b/t/db_dependent/Auth.t @@ -1384,17 +1384,25 @@ subtest 'StaffLoginBranchBasedOnIP' => sub { ); t::lib::Mocks::mock_preference( 'AutoLocation', 0 ); - my $other_branch = $builder->build_object( { class => 'Koha::Libraries', value => { branchip => "127.0.0.1" } } ); + my $other_branch = $builder->build_object( + { + class => 'Koha::Libraries', + value => { branchip => "127.0.0.1", branchcode => substr( "z" . $branch->branchcode, 0, 10 ) } + } + ); ( $userid, $cookie, $sessionID, $flags ) = C4::Auth::checkauth( $cgi, 0, { catalogue => 1 }, 'intranet' ); is( $userid, $patron->userid, "User successfully logged in" ); $session = C4::Auth::get_session($sessionID); - is( $session->param('branch'), $branch->branchcode, "Logged in branch is set based which branch when two libraries have same IP?" ); + is( + $session->param('branch'), $branch->branchcode, + "Logged in branch is set based which branch when two libraries have same IP?" + ); }; subtest 'AutoLocation' => sub { - plan tests => 11; + plan tests => 12; $schema->storage->txn_begin; @@ -1471,19 +1479,37 @@ subtest 'AutoLocation' => sub { ( $userid, $cookie, $sessionID, $flags, $template ) = C4::Auth::checkauth( $cgi, 0, { catalogue => 1 }, 'intranet' ); $session = C4::Auth::get_session($sessionID); - is( $session->param('branch'), $noip_library->branchcode, "When a branch with no IP set is chosen, we respect the choice regardless of current IP" ); + is( + $session->param('branch'), $noip_library->branchcode, + "When a branch with no IP set is chosen, we respect the choice regardless of current IP" + ); $ENV{REMOTE_ADDR} = '129.0.0.1'; # Set current IP to match other_branch - $cgi->param( 'branch', undef ); # Do not pass a branch + $cgi->param( 'branch', '' ); # Do not pass a branch $patron->library->branchip('')->store; # Unset user branch IP, to allow IP matching on any branch - # Add a second branch with same IP - my $another_library = $builder->build_object( { class => 'Koha::Libraries', value => { branchip => '129.0.0.1' } } ); + # Add a second branch with same IP + my $another_library = $builder->build_object( + { + class => 'Koha::Libraries', + value => { branchip => "129.0.0.1", branchcode => substr( "z" . $other_library->branchcode, 0, 10 ) } + } + ); ( $userid, $cookie, $sessionID, $flags, $template ) = C4::Auth::checkauth( $cgi, 0, { catalogue => 1 }, 'intranet', undef, undef, { do_not_print => 1 } ); $session = C4::Auth::get_session($sessionID); is( - $session->param('branch'), $other_library->branchcode, - "Which branch is chosen when home branch has no IP and more than 1 library matches?" + $session->param('branch'), $patron->library->branchcode, + "When user branch has no IP, and no branch chosen, user is logged in to their homebranch" + ); + + $cgi->param( 'branch', $another_library->branchcode ) + ; # Choose branch with duplicate IP and alphabetically later branchcode + ( $userid, $cookie, $sessionID, $flags, $template ) = + C4::Auth::checkauth( $cgi, 0, { catalogue => 1 }, 'intranet', undef, undef, { do_not_print => 1 } ); + $session = C4::Auth::get_session($sessionID); + is( + $session->param('branch'), $another_library->branchcode, + "When there is an IP conflict, we use the chosen branch if it matches" ); $schema->storage->txn_rollback; -- 2.39.5