Bug 14407 - Limit web-based self-checkout to specific IP addresses
Summary: Limit web-based self-checkout to specific IP addresses
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Self checkout (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Assignee: Nicholas van Oudtshoorn
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 16920
Blocks: 22943
  Show dependency treegraph
 
Reported: 2015-06-18 08:34 UTC by Nicholas van Oudtshoorn
Modified: 2020-01-06 20:14 UTC (History)
14 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This new feature allows you to increase the security of your online self checkout facilities by allowing you to limit their operation to a specified IP address or address range.
Version(s) released in:
19.05.00


Attachments
Allow web-based self-checkout to be limited to only certain IPs or IP ranges (7.16 KB, patch)
2015-06-25 04:49 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
Allow web-based self-checkout to be limited to only certain IPs or IP ranges (7.01 KB, patch)
2015-06-25 05:42 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
Bug 14407 - Allow web-based self checkout to be limited by IP addresses/ranges (6.84 KB, patch)
2015-07-09 02:06 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
Bug 14407 - Allow web-based self checkout to be limited by IP addresses/ranges (7.38 KB, patch)
2015-07-09 06:56 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
Limit web-based self-checkout to specific IP addresses (9.83 KB, patch)
2015-12-22 02:30 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
Limit web-based self-checkout to specific IP addresses (8.37 KB, patch)
2015-12-22 05:15 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
Limit web-based self-checkout to specific IP addresses (8.37 KB, patch)
2015-12-22 05:18 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
Bug 14407 - rebased to latest git (9.61 KB, patch)
2016-05-26 02:42 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
New patch, with silly typo fixed (9.61 KB, patch)
2016-05-26 05:10 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
New patch, addressing the SQL bug (9.81 KB, patch)
2016-07-28 04:15 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
Latest version of patch, with silly typo fixed (8.58 KB, patch)
2016-08-18 07:08 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
Latest version of patch, with errors fixed (8.68 KB, patch)
2016-09-22 08:00 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
Bug 14407 - Allow restricting SCO to IP or IP range Fix presence of tabs in files, and printslip refering to $query instead of $input (8.75 KB, patch)
2016-10-06 05:32 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14407 - Allow restricting SCO to IP or IP range Fix presence of tabs in files, and printslip refering to $query instead of $input (8.77 KB, patch)
2016-10-06 05:38 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14407 - Allow restricting SCO to IP or IP range Fix presence of tabs in files, and printslip refering to $query instead of $input (8.76 KB, patch)
2016-10-06 06:15 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14407 - Allow restricting SCO to IP or IP range (10.73 KB, patch)
2017-10-30 08:19 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
Allow restricting SCO to IP or IP range (10.90 KB, patch)
2017-10-30 08:25 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
Allow restricting SCO to IP or IP range (10.90 KB, application/mbox)
2017-10-30 08:29 UTC, Nicholas van Oudtshoorn
Details
Bug 14407: Allow restricting SCO to IP or IP range (8.00 KB, patch)
2018-01-12 18:13 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14407: Unit tests for in_ipset (2.35 KB, patch)
2018-01-12 18:16 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14407: Follow up add missing atomic update (978 bytes, patch)
2018-03-27 14:45 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14407: Follow up to add test case and clean noise (2.76 KB, patch)
2018-03-27 14:46 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14407: Allow restricting SCO to IP or IP range (7.99 KB, patch)
2018-03-27 17:07 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14407: Unit tests for in_ipset (2.40 KB, patch)
2018-03-27 17:07 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14407: Follow up add missing atomic update (1.01 KB, patch)
2018-03-27 17:07 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14407: Follow up to add test case and clean noise (2.81 KB, patch)
2018-03-27 17:07 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14407: Allow restricting SCO to IP or IP range (8.13 KB, patch)
2018-10-17 16:33 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14407: Unit tests for in_ipset (2.41 KB, patch)
2018-10-17 16:33 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14407: Follow up add missing atomic update (1.01 KB, patch)
2018-10-17 16:33 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14407: Follow up to add test case and clean noise (2.83 KB, patch)
2018-10-17 16:33 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14407: Follow up to add test case and clean noise (2.89 KB, patch)
2019-03-06 01:32 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 14407: Allow restricting SCO to IP or IP range (8.22 KB, patch)
2019-04-26 08:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14407: Unit tests for in_ipset (2.50 KB, patch)
2019-04-26 08:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14407: Follow up add missing atomic update (1.11 KB, patch)
2019-04-26 08:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14407: Follow up to add test case and clean noise (2.98 KB, patch)
2019-04-26 08:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14407: (RM follow-up) Add dependency for Net::CIDR (743 bytes, patch)
2019-04-30 11:54 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicholas van Oudtshoorn 2015-06-18 08:34:42 UTC
Web-based self-checkout is fantastic. But it'd be nice to have it a bit more locked down. Currently, anyone who knows/can guess the right URL can access it, even from offsite. I have a proof of concept hack, soon to be a patch, that fixes this. 
The hack, at the top of the sco*.pl files, something like

my $restricttoip = '192.168.1.30';
unless ($restricttoip eq $ENV{'REMOTE_ADDR'}) {
    print $query->redirect("/cgi-bin/koha/opac-main.pl");
    exit;
}
Comment 1 Nicholas van Oudtshoorn 2015-06-25 04:49:13 UTC Comment hidden (obsolete)
Comment 2 Indranil Das Gupta 2015-06-25 05:28:23 UTC
Comment on attachment 40612 [details] [review]
Allow web-based self-checkout to be limited to only certain IPs or IP ranges

Review of attachment 40612 [details] [review]:
-----------------------------------------------------------------

I've not tested the code, but looking at the patch, here are my preliminary comments.

::: installer/data/mysql/atomicupdate/selfcheckallowbyiprange.sql
@@ +1,1 @@
> +INSERT INTO systempreferences (variable,value,explanation,options,type) VALUES

two things here:

1/ you should probably consider re-naming the atomic update .sql filename - refer to http://wiki.koha-community.org/wiki/Database_updates#If_you_are_a_developer for file naming convention

2/ consider changing the syntax fro 'INSERT INTO' to 'INSERT IGNORE INTO' for compliance with Bug 9071. Otherwise this will likely result in "Failed QA"

::: installer/data/mysql/sysprefs.sql
@@ +55,4 @@
>  ('AutoSelfCheckAllowed','0','','For corporate and special libraries which want web-based self-check available from any PC without the need for a manual staff login. Most libraries will want to leave this turned off. If on, requires self-check ID and password to be entered in AutoSelfCheckID and AutoSelfCheckPass sysprefs.','YesNo'),
>  ('AutoSelfCheckID','','','Staff ID with circulation rights to be used for automatic web-based self-check. Only applies if AutoSelfCheckAllowed syspref is turned on.','free'),
>  ('AutoSelfCheckPass','','','Password to be used for automatic web-based self-check. Only applies if AutoSelfCheckAllowed syspref is turned on.','free'),
> +('SelfCheckAllowByIPRanges','',NULL,('Leave blank if not used. Use ranges or simple ip addresses separated by spaces, like <code>192.168.1.1 192.168.0.0/24</code>.)','Short'),

The sysprefs are ordered alphabetically (and not necessarily grouped together by functionality). Please fix this, otherwise this will get stuck in QA.

::: opac/sco/sco-main.pl
@@ +36,4 @@
>  
>  use CGI qw ( -utf8 );
>  use Digest::MD5 qw(md5_base64);
> +use Net::IP::Match::XS;

you are introducing a new perl dep. from my preliminary look, this module is probably not packaged for Debian / Ubuntu. If that is the case, then until it is packaged as .deb (upstream in the distros or by someone here or by Robin Sheat (provided he has the time and inclination to package it), this patch won't make it to a release. Just an FYI.
Comment 3 Nicholas van Oudtshoorn 2015-06-25 05:42:27 UTC Comment hidden (obsolete)
Comment 4 Robin Sheat 2015-06-28 23:49:20 UTC
The only way we do this currently (AFAIK) is by regex style matching, see how OpacSuppressionByIPRange is used in opac-search.pl.

A proper matching, like done in this bug, is probably a better way to do it.

However, I'd like to avoid using ::XS modules if possible, mostly because they will require custom compilation for every platform that people could possibly use, and I know that I don't have access to a s390x architecture machine to test on :)

Instead, is it possible to use Net::IP::Match, which also isn't in Debian, but will be substantially easier to build.
Comment 5 Nick Clemens 2015-06-30 17:26:21 UTC
Setting as assigned for response to comment #4
Comment 6 Nicholas van Oudtshoorn 2015-07-09 02:06:50 UTC Comment hidden (obsolete)
Comment 7 Robin Sheat 2015-07-09 02:27:44 UTC
(In reply to Nicholas van Oudtshoorn from comment #6)
> As per Robin's suggestion in Comment 4, it no longer uses an XS module. I
> have, however, gone for Net::CIDR instead of Net::IP::Match. For one thing,
> it looks like it's already packaged for Debian (and, as a bonus, for Fedora
> too!) For another, Net::IP::Match seemed to want to know a list of addresses
> etc. to match at "compile time".

Brilliant, Net::CIDR has been in Debian since the dawn of time, so that makes me happier :)

You've missed updating the use lines in sco-patron-image.pl though.

Also don't forget to update C4/Installer/PerlDependencies.pm (keep an eye on the version version numbers being correct, wheezy has 0.15.)

> If this patch is acceptable, would another to adjust opac-search to use
> Net::CIDR be a nice "tidying to one standard" patch?

I'd make a new bug for that, it'll possibly be more complicated than you expect as it'll have to account for the possibility of the format of a syspref changing, and either correcting that in the database or accepting the old form as well as the new one.
Comment 8 Nicholas van Oudtshoorn 2015-07-09 06:56:14 UTC Comment hidden (obsolete)
Comment 9 Robin Sheat 2015-07-09 23:12:46 UTC
As far as I can tell, this patch is looking good to me from a packaging-related POV.

(In reply to Nicholas van Oudtshoorn from comment #8)
> Agreed it would need a new bug. Have had a quick squiz, and it seems that
> NET::CIDR::range2cidr is perfectly happy with the format required by the
> current preference. It could, however, be changed to allow for much greater
> variety... (multiple subnets, individual IPs, etc.)

If it's backwards compatible, then that's the best of all worlds. I'd make a new bug for that, it certainly would be nice to gain a bit of standardisation.
Comment 10 Nicholas van Oudtshoorn 2015-07-14 04:53:16 UTC
Thanks Robin!

In that case, I'll change it to "needs signoff"!
Comment 11 Frédéric Demians 2015-11-11 15:30:11 UTC
Implementation note: Some code seems to be repeated in various places:

my @allowedipranges = split(' ', C4::Context->preference('SelfCheckAllowByIPRanges'));
if (scalar @allowedipranges > 0) {
    unless (Net::CIDR::cidrlookup($ENV{'REMOTE_ADDR'}, Net::CIDR::range2cidr(@allowedipranges))) {
        # action
        exit;
    }
}

Couldn't it be a good candidate to a new function in C4::Auth? Or even a new parameter to C4::Auth::get_template_and_user()?
Comment 12 Marc Véron 2015-12-16 19:30:09 UTC
Sorry, patch does not apply:

Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules
Merge conflict in C4/Installer/PerlDependencies.pm
Comment 13 Frédéric Demians 2015-12-16 20:04:58 UTC
I'm interested in this functionality. Without response from Nicholas van Oudtshoorn, I will refactor his patch.
Comment 14 Nicholas van Oudtshoorn 2015-12-22 02:30:21 UTC Comment hidden (obsolete)
Comment 15 Nicholas van Oudtshoorn 2015-12-22 05:15:35 UTC Comment hidden (obsolete)
Comment 16 Nicholas van Oudtshoorn 2015-12-22 05:18:59 UTC Comment hidden (obsolete)
Comment 17 Marc Véron 2016-02-10 17:44:41 UTC
Sorry, patch does not apply:

Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
Auto-merging installer/data/mysql/sysprefs.sql
Auto-merging C4/Installer/PerlDependencies.pm
CONFLICT (content): Merge conflict in C4/Installer/PerlDependencies.pm
Auto-merging C4/Auth.pm
Failed to merge in the changes.


Additionally, I think that the new function in C4/Auth.pm would need some testing in t/db_dependent/Auth.t
Comment 18 Nicholas van Oudtshoorn 2016-05-26 02:42:55 UTC Comment hidden (obsolete)
Comment 19 Nicholas van Oudtshoorn 2016-05-26 05:10:57 UTC Comment hidden (obsolete)
Comment 20 Nicholas van Oudtshoorn 2016-07-14 03:49:38 UTC
Pinging anyone interested in looking at gettting this signed off! :-)
Comment 21 Marc Véron 2016-07-14 09:57:48 UTC
> 2) In your mysql client use your Koha database and execute:
>     > DELETE FROM systempreferences;
>     > SOURCE ~/kohaclone/installer/data/mysql/sysprefs.sql;
>     -- Should be no errors.

SOURCE: 
- First I had an error related to Bug 16920.
- With this patch applied on top of 16920, I get following error:
  ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '' at line 546
Comment 22 Marc Véron 2016-07-14 12:08:41 UTC
Found in sysprefs.sql
('SelfCheckAllowByIPRanges','',NULL,('Leave blank if not used. Use ranges or simple ip addresses separated by spaces, like <code>192.168.1.1 192.168.0.0/24</code>.)','Short'),

NULL,('Leave blank...
should be
NULL,'(Leave blank...
Comment 23 Nicholas van Oudtshoorn 2016-07-28 04:15:41 UTC Comment hidden (obsolete)
Comment 24 Marc Véron 2016-07-28 05:32:51 UTC
Quick test:

SQL error is gone.

However, I get an 'Internal Server Error'
plack-error.log says:

Could not compile (...)/opac/sco/sco-main.pl: "in_ipset" is not exported by the C4::Auth module
Can't continue after import errors at (...)/opac/sco/sco-main.pl line 40.

(I have not to much time for testing until mid August.)
Comment 25 Nicholas van Oudtshoorn 2016-08-18 07:08:52 UTC
Created attachment 54560 [details] [review]
Latest version of patch, with silly typo fixed

Hi Marc,

Thanks for that. Can't believe I didn't put the fix for that into the patch here - we've been running with it for ages! :-) 

This version of the patch should have fixed it (and also rebased to master so that it will apply!)
Comment 26 Marc Véron 2016-08-18 09:52:32 UTC
Hi Nicholas,

I tested and acces was restricted as expected (except printslip.pl, see below)

QA-Tools complain:

 FAIL	opac/sco/help.pl
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 34)
		forbidden pattern: tab char (line 35)

 FAIL	opac/sco/printslip.pl
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 39)
		forbidden pattern: tab char (line 40)

   FAIL	  valid
		Global symbol "$query" requires explicit package name 
		opac/sco/printslip.pl had compilation errors.

 FAIL	opac/sco/sco-main.pl
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 60)
		forbidden pattern: tab char (line 61)
		forbidden pattern: tab char (line 62)

 FAIL	opac/sco/sco-patron-image.pl
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 40)
		forbidden pattern: tab char (line 39)
Comment 27 Nicholas van Oudtshoorn 2016-09-22 08:00:26 UTC
Created attachment 55746 [details] [review]
Latest version of patch, with errors fixed

Thanks for that Marc. Hopefully all of those mistakes have now been rectified! :-)
Comment 28 Marc Véron 2016-09-22 08:58:52 UTC
Hi Nicholas

The qa tool no longer complains.

I re-tested with following scenario (restart plack after each change to syspref):

- Client's IP is 192.168.1.32

- SelfCheckAllowByIPRanges is empty
  go to  .../cgi-bin/koha/sco/sco-main.pl OK for all IPs
  
- Set SelfCheckAllowByIPRanges to 192.168.1.32
  Allowed for client OK
  Client with other IP redirected to OPAC main page OK

- Set SelfCheckAllowByIPRanges to 192.168
  Allowed for client OK
  Client with other IP redirected to OPAC main page OK

- Set SelfCheckAllowByIPRanges to 192.168.1.32 192.168.1.30 192.168.1.10
  Allowed for client OK
  Client with other IP redirected to OPAC main page OK

- Set SelfCheckAllowByIPRanges to 192.168.1.15 192.168.1.30 192.168.1.10
  Client redirected to OPAC main page OK
  Client with other IP redirected to OPAC main page OK 

- Set SelfCheckAllowByIPRanges to 192.168.1.1 192.168.1.0/36
  as described for syspref SelfCheckAllowByIPRanges
  Internal Server Error
  Plack error log says;
  36, as in '192.168.1.30/36', does not make sense at /home/marc/koha/C4/Auth.pm line 2058.

Is it a bug or a wrong description for SelfCheckAllowByIPRanges?
Comment 29 Nicholas van Oudtshoorn 2016-10-06 02:23:12 UTC
Hmmm - I don't see SelfCheckAllowByIPRanges preferencing suggesting one use /36. The library I've used should support something like /24, which is what the preference gives as a suggestion. Is /36 even a legal CIDR for ipv4? (I wouldn't have thought so - you'd be masking out more than the ipv4 address allows, I think...)
Comment 30 Marc Véron 2016-10-06 05:30:53 UTC
(In reply to Nicholas van Oudtshoorn from comment #29)
> Hmmm - I don't see SelfCheckAllowByIPRanges preferencing suggesting one use
> /36. The library I've used should support something like /24, which is what
> the preference gives as a suggestion. Is /36 even a legal CIDR for ipv4? (I
> wouldn't have thought so - you'd be masking out more than the ipv4 address
> allows, I think...)

Sorry, my mistake...

- Set SelfCheckAllowByIPRanges to 192.168.1.1 192.168.1.32/24
  Allowed for clients with such IP's OK
  Client with other IP redirected to OPAC main page OK
Comment 31 Marc Véron 2016-10-06 05:32:37 UTC
Created attachment 56064 [details] [review]
Bug 14407 - Allow restricting SCO to IP or IP range Fix presence of tabs in files, and printslip refering to $query instead of $input

Tested with scenarios above, works OK

Signed-off-by: Marc <veron@veron.ch>
Comment 32 Marc Véron 2016-10-06 05:38:52 UTC
Created attachment 56065 [details] [review]
Bug 14407 - Allow restricting SCO to IP or IP range Fix presence of tabs in files, and printslip refering to $query instead of $input

Tested two patches together, with scenario above. Works OK
Signed-off-by: Marc <veron@veron.ch>
Comment 33 Marc Véron 2016-10-06 05:52:05 UTC
Comment on attachment 55746 [details] [review]
Latest version of patch, with errors fixed

Sorry for the noise - made mistakes with the patches while signing off.
Reset...
Comment 34 Marc Véron 2016-10-06 06:15:13 UTC
Created attachment 56066 [details] [review]
Bug 14407 - Allow restricting SCO to IP or IP range Fix presence of tabs in files, and printslip refering to $query instead of $input

Tested with scenarios above, works as expected.
Signed-off-by: Marc <veron@veron.ch>
Comment 35 Marcel de Rooy 2016-10-20 10:40:13 UTC
(In reply to Marc Véron from comment #17)
> Additionally, I think that the new function in C4/Auth.pm would need some
> testing in t/db_dependent/Auth.t

Nicholas,
Please add some unit tests.

Changing status to reflect need for change.
Comment 36 Nicholas van Oudtshoorn 2017-10-30 08:19:20 UTC
Created attachment 68834 [details] [review]
Bug 14407 - Allow restricting SCO to IP or IP range

Added some unit tests to this. Not added in the db_dependent directory, given that the new function (in_ipset) doesn't actually touch the database at all. It's meant to be a function that can be reused in the future for other ip based limits in Koha.
Comment 37 Nicholas van Oudtshoorn 2017-10-30 08:19:54 UTC
Added unit tests for in_ipset
Comment 38 Nicholas van Oudtshoorn 2017-10-30 08:25:46 UTC
Created attachment 68835 [details] [review]
Allow restricting SCO to IP or IP range

Sorry for the noise - running on two git instances, and forgot to include the required updates to Auth.pm that testing revealed. Now fails secure if invalid IP ranges are set...
Comment 39 Nicholas van Oudtshoorn 2017-10-30 08:29:15 UTC
Created attachment 68836 [details]
Allow restricting SCO to IP or IP range

Ah no! One more bit of noise - a very minor copy-and-paste mistake... in the copyright statement in the new unit test Auth.t!
Hopefully this is now it!
Comment 40 Mark Tompsett 2018-01-12 18:13:54 UTC
Created attachment 70475 [details] [review]
Bug 14407: Allow restricting SCO to IP or IP range

Converted this to actual applicable patches.
I think the test plan is comment #28. -- Mark Tompsett
Comment 41 Mark Tompsett 2018-01-12 18:16:34 UTC
Created attachment 70476 [details] [review]
Bug 14407: Unit tests for in_ipset

TEST PLAN
---------
1) apply all pathches
2) sudo koha-shell -c bash kohadev
   prove t/Auth.t
   -- should run.
3) run koha qa test tools
Comment 42 Mark Tompsett 2018-01-12 18:18:18 UTC
Comment on attachment 68836 [details]
Allow restricting SCO to IP or IP range

Patch attached in a non-patch way making things harder to git bz.
Comment 43 Nicholas van Oudtshoorn 2018-01-23 09:20:55 UTC
Thanks for fixing that - my bad :-(
Comment 44 Mark Tompsett 2018-03-27 13:06:14 UTC
Comment on attachment 70476 [details] [review]
Bug 14407: Unit tests for in_ipset

Review of attachment 70476 [details] [review]:
-----------------------------------------------------------------

::: t/Auth.t
@@ +18,5 @@
> +use Modern::Perl;
> +use Test::More tests => 10;
> +
> +use C4::Auth qw / in_ipset /;
> +use warnings;

Modern::Perl should cover the use warnings situation.
Comment 45 Mark Tompsett 2018-03-27 14:45:03 UTC
Created attachment 73316 [details] [review]
Bug 14407: Follow up add missing atomic update

The patch only added the system preference for fresh installs.
Comment 46 Mark Tompsett 2018-03-27 14:46:25 UTC
Created attachment 73318 [details] [review]
Bug 14407: Follow up to add test case and clean noise

Comment #28 has a /36 which is invalid CIDR.
This triggers a crash and noise.
This cleans up the crash and noise, and adds
test cases to check for them.

prove t/Auth.t
-- before missing null case, and /36 case.
-- after null case, and /36 with/without warnings.
Comment 47 Mark Tompsett 2018-03-27 17:00:06 UTC
kohadevbox is triggered from 192.168.50.1, so testing 192.168.0.0/16 worked nicely. :)
Comment 48 Mark Tompsett 2018-03-27 17:02:03 UTC
(In reply to Marc Véron from comment #28)
> - Set SelfCheckAllowByIPRanges to 192.168.1.1 192.168.1.0/36
>   as described for syspref SelfCheckAllowByIPRanges
>   Internal Server Error
>   Plack error log says;
>   36, as in '192.168.1.30/36', does not make sense at
> /home/marc/koha/C4/Auth.pm line 2058.
> 
> Is it a bug or a wrong description for SelfCheckAllowByIPRanges?

36 is not valid. 8+8+8+8 = 32. Why would you have a number bigger than 32 on an IPv4 address?
Comment 49 Mark Tompsett 2018-03-27 17:07:43 UTC
Created attachment 73339 [details] [review]
Bug 14407: Allow restricting SCO to IP or IP range

Converted this to actual applicable patches.
I think the test plan is comment #28. -- Mark Tompsett

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 50 Mark Tompsett 2018-03-27 17:07:46 UTC
Created attachment 73340 [details] [review]
Bug 14407: Unit tests for in_ipset

TEST PLAN
---------
1) apply all pathches
2) sudo koha-shell -c bash kohadev
   prove t/Auth.t
   -- should run.
3) run koha qa test tools

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 51 Mark Tompsett 2018-03-27 17:07:50 UTC
Created attachment 73341 [details] [review]
Bug 14407: Follow up add missing atomic update

The patch only added the system preference for fresh installs.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 52 Mark Tompsett 2018-03-27 17:07:54 UTC
Created attachment 73342 [details] [review]
Bug 14407: Follow up to add test case and clean noise

Comment #28 has a /36 which is invalid CIDR.
This triggers a crash and noise.
This cleans up the crash and noise, and adds
test cases to check for them.

prove t/Auth.t
-- before missing null case, and /36 case.
-- after null case, and /36 with/without warnings.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 53 Mark Tompsett 2018-03-27 17:13:58 UTC
The question is does the in_ipset return the correct value for bad ranges? Because currently, it assumes open when a bad range is provided.
Comment 54 Michal Denar 2018-10-01 18:44:34 UTC
Please, rebase on master.
Comment 55 Mark Tompsett 2018-10-17 16:32:33 UTC
(In reply to Michal Denar from comment #54)
> Please, rebase on master.

Please remember to set to "Patch Does Not Apply" when you request a rebase. :)
Comment 56 Mark Tompsett 2018-10-17 16:33:42 UTC
Created attachment 80735 [details] [review]
Bug 14407: Allow restricting SCO to IP or IP range

Converted this to actual applicable patches.
I think the test plan is comment #28. -- Mark Tompsett

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 57 Mark Tompsett 2018-10-17 16:33:47 UTC
Created attachment 80736 [details] [review]
Bug 14407: Unit tests for in_ipset

TEST PLAN
---------
1) apply all pathches
2) sudo koha-shell -c bash kohadev
   prove t/Auth.t
   -- should run.
3) run koha qa test tools

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 58 Mark Tompsett 2018-10-17 16:33:53 UTC
Created attachment 80737 [details] [review]
Bug 14407: Follow up add missing atomic update

The patch only added the system preference for fresh installs.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 59 Mark Tompsett 2018-10-17 16:33:58 UTC
Created attachment 80738 [details] [review]
Bug 14407: Follow up to add test case and clean noise

Comment #28 has a /36 which is invalid CIDR.
This triggers a crash and noise.
This cleans up the crash and noise, and adds
test cases to check for them.

prove t/Auth.t
-- before missing null case, and /36 case.
-- after null case, and /36 with/without warnings.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 60 Hayley Pelham 2019-03-06 01:32:34 UTC
Created attachment 86123 [details] [review]
Bug 14407: Follow up to add test case and clean noise

Comment #28 has a /36 which is invalid CIDR.
This triggers a crash and noise.
This cleans up the crash and noise, and adds
test cases to check for them.

prove t/Auth.t
-- before missing null case, and /36 case.
-- after null case, and /36 with/without warnings.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
Looked signed off but status was Needs Signoff so I tested it again.
Comment 61 Marcel de Rooy 2019-04-26 06:58:27 UTC
QA: Looking here
Comment 62 Marcel de Rooy 2019-04-26 08:24:02 UTC
Created attachment 88863 [details] [review]
Bug 14407: Allow restricting SCO to IP or IP range

Converted this to actual applicable patches.
I think the test plan is comment #28. -- Mark Tompsett

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 63 Marcel de Rooy 2019-04-26 08:24:07 UTC
Created attachment 88864 [details] [review]
Bug 14407: Unit tests for in_ipset

TEST PLAN
---------
1) apply all pathches
2) sudo koha-shell -c bash kohadev
   prove t/Auth.t
   -- should run.
3) run koha qa test tools

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 64 Marcel de Rooy 2019-04-26 08:24:12 UTC
Created attachment 88865 [details] [review]
Bug 14407: Follow up add missing atomic update

The patch only added the system preference for fresh installs.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 65 Marcel de Rooy 2019-04-26 08:24:17 UTC
Created attachment 88866 [details] [review]
Bug 14407: Follow up to add test case and clean noise

Comment #28 has a /36 which is invalid CIDR.
This triggers a crash and noise.
This cleans up the crash and noise, and adds
test cases to check for them.

prove t/Auth.t
-- before missing null case, and /36 case.
-- after null case, and /36 with/without warnings.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 66 Marcel de Rooy 2019-04-26 08:31:21 UTC
@RM: Do we still have a keyword for packaging? dependency? Net::CIDR is added, but it seems to be installed already with koha-common and friends.

The pref allows both start-finish and CIDR IP ranges. Although formally piping the pref into range2cidr converts start-finish ranges into CIDR. Both work.

The patch uses CGI header(status=> ...) notation. The codebase also contains the older -status notation. But CGI::Util handles both.
Comment 67 Nick Clemens 2019-04-26 10:46:38 UTC
I think dependency works, I will also email Mirko just for confirmation

I added a see also to bug 21477 - this is just another case of a thing we already do so no blocker, just mentioning

Once Mirko confirms I think this one is good
Comment 68 Mirko Tietgen 2019-04-26 11:24:57 UTC
libnet-server-perl depends on libnet-cidr-perl so we already have it. But it should be declarded as dependency for koha-common for this bug.
Comment 69 Mirko Tietgen 2019-04-26 14:00:01 UTC
(Please declare the dependency in PerlDependencies, I'll do the rest.)
Comment 70 Nick Clemens 2019-04-30 11:54:28 UTC
Created attachment 89114 [details] [review]
Bug 14407: (RM follow-up) Add dependency for Net::CIDR
Comment 71 Nick Clemens 2019-04-30 11:55:09 UTC
Can I just get a thumbs up on adding the dep before pushing, I tried to get the version number in Jessie and use that for the minimum
Comment 72 Martin Renvoize 2019-05-10 11:37:07 UTC
Dependency is already added in the first patch... though it specifies 0.15 as minumum whereas you're specifies 0.19.  I think I'd go with 0.17 (as the most recently listed for Jessie here: https://packages.debian.org/search?searchon=names&keywords=libnet-cidr-perl).

Otherwise.. I think we're good to go.
Comment 73 Nick Clemens 2019-05-10 12:24:45 UTC
Awesome work all!

Pushed to master for 19.05
Comment 74 Martin Renvoize 2019-05-17 13:20:48 UTC
New feature will not be backported to 18.11.x series.
Comment 75 David Cook 2019-05-21 01:42:29 UTC
This is cool, but it's unfortunate the name "ipset" was used, since that's an actual tool that can be used for managing IP sets in the Linux kernel: http://ipset.netfilter.org/ipset.man.html

When I saw the function name "in_ipset", I was confused until I saw that we weren't actually using ipset and it was just coincidental naming.

Not a criticism. Just sharing my initial confusion with the naming.
Comment 76 Martin Renvoize 2019-05-21 06:09:07 UTC
We've not released yet.. perhaps we could change it to in_iprange quickly before it's set in stone forever more.

What do you think David, does that sound more appropriate?
Comment 77 David Cook 2019-05-22 06:03:12 UTC
(In reply to Martin Renvoize from comment #76)
> We've not released yet.. perhaps we could change it to in_iprange quickly
> before it's set in stone forever more.
> 
> What do you think David, does that sound more appropriate?

If folk are willing, I'd say yes to changing it. How about "in_ip_ranges"?

If we look at the current "sub in_ipset", we see the following line:

my @allowedipranges = split(' ', $ipset);

Probably makes sense to use "ranges" or "range_list" since that's the terminology used by Net::CIDR as well: https://metacpan.org/pod/Net::CIDR#@cidr_list=Net::CIDR::range2cidr(@range_list);

(Actually, if I really wanted to split hairs, I'd say "@allowedipranges" should probably be changed to "@ip_ranges", as theoretically this function could also be used to interrogate a list of IP ranges to block. Yay code re-use!)
Comment 78 David Cook 2019-05-22 06:06:46 UTC
Actually, looking at "sub in_ipset" again, I'd also invert the logic, so that the default return value is 0.

That way, if you've misconfigured the value of $ipset, you're not going to accidentally return 1. 

Also, using $ENV{'REMOTE_ADDR'} won't work properly if you're using a reverse proxy. I don't know how we handle this with Plack, but for CGI users running behind reverse proxies, there's no way this is going to work correctly.