From a9759138a763b580940317db7671c30fa2a718c1 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 --- 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 f9484add7ce..a1019737c08 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 7b226ea8762..efba78026b7 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 a721da0a8ba..bf092f87272 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.45.1