From 91cb3d35973d2cb1d1e21a896143817383ad18e5 Mon Sep 17 00:00:00 2001 From: Michael Hafen Date: Fri, 2 Jul 2021 11:03:08 -0600 Subject: [PATCH] Bug 28657: expand branches.branchip to allow for multiple ip ranges. Content-Type: text/plain; charset="utf-8" change branches.branchip to a mediumtext, and remove '*' from existing branchip settings. Change C4/Auth.pm from a regexp to using in_iprange() to check client address against branchip. (in_iprange() doesn't work with '*' in branchip.) Change in_iprange() to default to false (which is what the POD says it should do). Change note in admin/branches.tt under branchip field to reflect changes to branchip handling. Bug 28657: (QA follow-up) Return 1 if no range, improve unit tests Bug 28657: (Follow-up for QA) Tidy the code and make it more consident remove branches.tt branchip input maxlength attribute. Have the db_rev match the kohastructure.sql for column collation and comment. Use new style db_rev skeleton. Fix comment C4::Auth. Have Koha::Library->store() watch for and remove '*' from branchip. Add a couple more test for full coverage. Test plan: 1. Turn on AutoLocation system preference 2. Change a branches branchip to multiple ip ranges seperated by a space, making sure one of the ranges includes your computers address. 3. Log out and try logging in. The location check will fail. 4. Apply patch. Restart plack if necessary. 5. Try logging in again. The location check will succeed. --- C4/Auth.pm | 51 +++++++++++-------- Koha/Library.pm | 6 +++ .../data/mysql/atomicupdate/bug_28657.pl | 26 ++++++++++ installer/data/mysql/kohastructure.sql | 2 +- .../prog/en/modules/admin/branches.tt | 4 +- t/Auth.t | 28 +++++----- 6 files changed, 80 insertions(+), 37 deletions(-) create mode 100755 installer/data/mysql/atomicupdate/bug_28657.pl diff --git a/C4/Auth.pm b/C4/Auth.pm index c25cf61876..efc518c93e 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -1197,16 +1197,19 @@ sub checkauth { # we have to check they are coming from the right ip range my $domain = $branches->{$branchcode}->{'branchip'}; - $domain =~ s|\.\*||g; - if ( $ip !~ /^$domain/ ) { + next unless ($domain); + if ( !in_iprange($domain) ) { $loggedin = 0; - $cookie = $cookie_mgr->replace_in_list( $cookie, $query->cookie( - -name => 'CGISESSID', - -value => '', - -HttpOnly => 1, - -secure => ( C4::Context->https_enabled() ? 1 : 0 ), - -sameSite => 'Lax', - )); + $cookie = $cookie_mgr->replace_in_list( + $cookie, + $query->cookie( + -name => 'CGISESSID', + -value => '', + -HttpOnly => 1, + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), + -sameSite => 'Lax', + ) + ); $info{'wrongip'} = 1; } } @@ -1215,11 +1218,12 @@ sub checkauth { # now we work with the treatment of ip my $domain = $branches->{$br}->{'branchip'}; - if ( $domain && $ip =~ /^$domain/ ) { + next unless ($domain); + if ( in_iprange($domain) ) { $branchcode = $branches->{$br}->{'branchcode'}; # new op dev : add the branchname to the cookie - $branchname = $branches->{$br}->{'branchname'}; + $branchname = $branches->{$br}->{'branchname'}; } } @@ -1651,18 +1655,19 @@ sub check_api_auth { if ( $query->param('branch') ) { $branchcode = $query->param('branch'); my $library = Koha::Libraries->find($branchcode); - $branchname = $library? $library->branchname: ''; + $branchname = $library ? $library->branchname: ''; } my $branches = { map { $_->branchcode => $_->unblessed } Koha::Libraries->search->as_list }; foreach my $br ( keys %$branches ) { # now we work with the treatment of ip my $domain = $branches->{$br}->{'branchip'}; - if ( $domain && $ip =~ /^$domain/ ) { + next unless ($domain); + if ( in_iprange($domain) ) { $branchcode = $branches->{$br}->{'branchcode'}; # new op dev : add the branchname to the cookie - $branchname = $branches->{$br}->{'branchname'}; + $branchname = $branches->{$br}->{'branchname'}; } } $session->param( 'number', $borrowernumber ); @@ -2315,14 +2320,18 @@ Returns 1 if the remote address is in the provided iprange, or 0 otherwise. sub in_iprange { my ($iprange) = @_; - my $result = 1; - my @allowedipranges = $iprange ? split(' ', $iprange) : (); - if (scalar @allowedipranges > 0) { + return 1 unless $iprange; + my $result = 0; + + my @allowedipranges = $iprange ? split( ' ', $iprange ) : (); + if (@allowedipranges) { my @rangelist; - eval { @rangelist = Net::CIDR::range2cidr(@allowedipranges); }; return 0 if $@; - eval { $result = Net::CIDR::cidrlookup($ENV{'REMOTE_ADDR'}, @rangelist) } || Koha::Logger->get->warn('cidrlookup failed for ' . join(' ',@rangelist) ); - } - return $result ? 1 : 0; + eval { @rangelist = Net::CIDR::range2cidr(@allowedipranges); }; + return 0 if $@; + eval { $result = Net::CIDR::cidrlookup( $ENV{'REMOTE_ADDR'}, @rangelist ) } + || Koha::Logger->get->warn( 'cidrlookup failed for ' . join( ' ', @rangelist ) ); + } + return $result ? 1 : 0; } sub getborrowernumber { diff --git a/Koha/Library.pm b/Koha/Library.pm index 2e9259b450..f8821b3055 100644 --- a/Koha/Library.pm +++ b/Koha/Library.pm @@ -58,6 +58,12 @@ sub store { $flush = 1 if ( $self_from_storage->branchname ne $self->branchname ); } + if ( $self->branchip =~ /\*/ ) { + my $branchip = $self->branchip; + $branchip =~ s/\*//g; + $branchip =~ s/\.{2,}/./g; + $self->branchip( $branchip ); + } $self = $self->SUPER::store; if ($flush) { diff --git a/installer/data/mysql/atomicupdate/bug_28657.pl b/installer/data/mysql/atomicupdate/bug_28657.pl new file mode 100755 index 0000000000..08382b266a --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_28657.pl @@ -0,0 +1,26 @@ +use Modern::Perl; + +return { + bug_number => "28657", + description => "expand branches.branchip to allow for multiple ip ranges and remove '*'", + up => sub { + my ($args) = @_; + my ( $dbh, $out ) = @$args{qw(dbh out)}; + + $dbh->do( + q{ALTER TABLE `branches` MODIFY `branchip` mediumtext DEFAULT NULL COMMENT 'the IP address(s) for your library or branch'} + ); + say $out "Modify column 'branches.branchip' to mediumtext"; + + my $sth_select = $dbh->prepare(q{SELECT branchip,branchcode FROM branches WHERE branchip like '%*%'}); + my $sth_update = $dbh->prepare(q{UPDATE branches SET branchip = ? WHERE branchcode = ?}); + $sth_select->execute; + my $results = $sth_select->fetchall_arrayref( {} ); + foreach (@$results) { + $_->{'branchip'} =~ s/\*//g; + $_->{'branchip'} =~ s/\.{2,}/./g; + $sth_update->execute( $_->{'branchip'}, $_->{'branchcode'} ); + } + say $out "Update column 'branches.branchip' to remove '*'"; + }, +}; diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql index add4518807..3fff501aca 100644 --- a/installer/data/mysql/kohastructure.sql +++ b/installer/data/mysql/kohastructure.sql @@ -1603,7 +1603,7 @@ CREATE TABLE `branches` ( `branchreturnpath` longtext DEFAULT NULL COMMENT 'the email to be used as Return-Path', `branchurl` longtext DEFAULT NULL COMMENT 'the URL for your library or branch''s website', `issuing` tinyint(4) DEFAULT NULL COMMENT 'unused in Koha', - `branchip` varchar(15) DEFAULT NULL COMMENT 'the IP address for your library or branch', + `branchip` mediumtext DEFAULT NULL COMMENT 'the IP address(s) for your library or branch', `branchnotes` longtext DEFAULT NULL COMMENT 'notes related to your library or branch', `geolocation` varchar(255) DEFAULT NULL COMMENT 'geolocation of your library', `marcorgcode` varchar(16) DEFAULT NULL COMMENT 'MARC Organization Code, see http://www.loc.gov/marc/organizations/orgshome.html, when empty defaults to syspref MARCOrgCode', diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/branches.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/branches.tt index 9f4b1e09e2..70b6dbab94 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/branches.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/branches.tt @@ -246,8 +246,8 @@
  • - -
    Can be entered as a single IP, or a subnet such as 192.168.1.*
    + +
    Can be entered as a single IP, an IP range such as 192.168.1. or 192.168.1.10-192.168.1.50 or 192.168.1.0/24, or multiple space separated IP addresses or ranges.
  • diff --git a/t/Auth.t b/t/Auth.t index d5b2f1e7fa..6881734583 100755 --- a/t/Auth.t +++ b/t/Auth.t @@ -16,22 +16,24 @@ # along with Koha; if not, see . use Modern::Perl; -use Test::More tests => 12; +use Test::More tests => 14; use Test::Warn; use C4::Auth qw( in_iprange ); $ENV{REMOTE_ADDR} = '192.168.1.30'; -ok(in_iprange("192.168.1.30"), 'simple single ip matching remote ip'); -ok(!in_iprange("192.168.1.31"), 'simple single ip not match remote ip'); -ok(in_iprange("192.168.1.1/24"), 'simple ip range/24 with remote ip in it'); -ok(!in_iprange("192.168.2.1/24"), 'simple ip range/24 with remote ip not in it'); -ok(in_iprange("192.168.2.1/16"), 'simple ip range/16 with remote ip in it'); -ok(!in_iprange("192.168.1.10-30"), 'invalidly represented IP range with remote ip in it'); -ok(in_iprange("192.168.1.10-192.168.1.30"), 'validly represented ip range with remote ip in it'); -ok(in_iprange("127.0.0.1 192.168.1.30 192.168.2.10-192.168.2.25"), 'multiple ips and ranges, including the remote ip'); -ok(!in_iprange("127.0.0.1 8.8.8.8 192.168.2.1/24 192.168.3.1/24 192.168.1.1-192.168.1.29"), "multiple ip and ip ranges, with the remote ip in none of them"); -ok(in_iprange(""), "blank list given, no preference set - implies everything goes through."); -ok(in_iprange(), "no list given, no preference set - implies everything goes through."); -ok(in_iprange("192.168.1.1/36"), 'simple invalid ip range/36 with remote ip in it'); +is( 1, in_iprange("192.168.1.30"), 'simple single ip matching remote ip' ); +is( 0, in_iprange("192.168.1.31"), 'simple single ip not match remote ip' ); +is( 1, in_iprange("192.168.1.1/24"), 'simple ip range/24 with remote ip in it' ); +is( 0, in_iprange("192.168.2.1/24"), 'simple ip range/24 with remote ip not in it' ); +is( 1, in_iprange("192.168.2.1/16"), 'simple ip range/16 with remote ip in it' ); +is( 0, in_iprange("192.168.1.10-30"), 'invalidly represented IP range with remote ip in it' ); +is( 1, in_iprange("192.168.1.10-192.168.1.30"), 'validly represented ip range with remote ip in it' ); +is( 1, in_iprange("127.0.0.1 192.168.1.30 192.168.2.10-192.168.2.25"), 'multiple ips and ranges, including the remote ip' ); +is( 0, in_iprange("127.0.0.1 8.8.8.8 192.168.2.1/24 192.168.3.1/24 192.168.1.1-192.168.1.29" ), "multiple ip and ip ranges, with the remote ip in none of them" ); +is( 1, in_iprange(""), "blank list given, no preference set - implies everything goes through." ); +is( 1, in_iprange(), "no list given, no preference set - implies everything goes through." ); +is( 0, in_iprange("192.168.1.1/36"), 'simple invalid ip range/36 with remote ip in it' ); +is( 1, in_iprange("192.168.1."), 'simple ip range missing last octet with remote ip in it' ); +is( 1, in_iprange("192.168.1.*"), 'simple ip range with "*" with remote ip in it. Undocumented feature; this should not work, but it does.' ); -- 2.34.1