From 46cecfdd72ee45bfad03c6875fdc078df48eabec 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 --- 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 f9484add7c..a1019737c0 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -1214,8 +1214,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'}; @@ -1248,7 +1248,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 7b226ea876..efba78026b 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 a721da0a8b..bf092f8727 100755 --- a/t/db_dependent/Auth.t +++ b/t/db_dependent/Auth.t @@ -1341,17 +1341,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; @@ -1428,19 +1436,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