[Bug 14407] New: Basic access restrictions for web-based self-checkout
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14407 Bug ID: 14407 Summary: Basic access restrictions for web-based self-checkout Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Self checkout Assignee: koha-bugs@lists.koha-community.org Reporter: vanoudt@gmail.com QA Contact: testopia@bugs.koha-community.org 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; } -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14407 Nicholas van Oudtshoorn <vanoudt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |vanoudt@gmail.com |ity.org | Status|NEW |Needs Signoff CC| |vanoudt@gmail.com --- Comment #1 from Nicholas van Oudtshoorn <vanoudt@gmail.com> --- Created attachment 40612 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=40612&action=edit Allow web-based self-checkout to be limited to only certain IPs or IP ranges Bug 14407 - Allow web-based self-checkout to be limited to only certain IPs or IP ranges Test plan: 0) Back up your databse 1) Apply this patch 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. > SELECT * FROM systempreferences WHERE variable='SelfCheckAllowByIPRanges'; -- Should see 1 entry > QUIT; 3) Restore your database 4) Run installer/data/mysql/atomicupdate/selfcheckallowbyiprange.sql 5) In your mysql client, using your Koha database, execute: > SELECT * FROM systempreferences WHERE variable='SelfCheckAllowByIPRanges'; -- should see the same entry as above 6) Try access the Self-checkout, self-checkout help, and self-checkout patron images. It should work. 7) Log into the staff client 8) Home -> Koha administration -> Global system preferences 9) -> CIRCULATION Make sure that SelfCheckAllowByIPRanges is set to a computer you can test on 10) Try access the Self-checkout, self-checkout help, and self-checkout patron images from a computer with an IP in the range above. It should work. 11) Try access the Self-checkout, self-checkout help, and self-checkout patron images from a computer with an IP outside of the range above. It should *not* work. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14407 Nicholas van Oudtshoorn <vanoudt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Basic access restrictions |Limit web-based |for web-based self-checkout |self-checkout to specific | |IP addresses -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14407 --- Comment #2 from Indranil Das Gupta <indradg@gmail.com> --- Comment on attachment 40612 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=40612 Allow web-based self-checkout to be limited to only certain IPs or IP ranges Review of attachment 40612: --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=14407&attachment=40612) ----------------------------------------------------------------- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14407 Indranil Das Gupta <indradg@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |indradg@gmail.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14407 Nicholas van Oudtshoorn <vanoudt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #40612|0 |1 is obsolete| | --- Comment #3 from Nicholas van Oudtshoorn <vanoudt@gmail.com> --- Created attachment 40613 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=40613&action=edit Allow web-based self-checkout to be limited to only certain IPs or IP ranges Fixes the comments raised in Comment #2. As for introducing a new perl module - does Koha already have one that is used for testing whether an IP is in a particular range or group of supplied IP addresses? We use Fedora here, so I have no idea what is/isn't in Debian... hopefully this patch can be switched to use an existing library, or else it won't be too much work to get it added to Koha... -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14407 Indranil Das Gupta <indradg@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |robin@catalyst.net.nz -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14407 --- Comment #4 from Robin Sheat <robin@catalyst.net.nz> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14407 Nick Clemens <nick@quecheelibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |ASSIGNED CC| |nick@quecheelibrary.org --- Comment #5 from Nick Clemens <nick@quecheelibrary.org> --- Setting as assigned for response to comment #4 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14407 Nicholas van Oudtshoorn <vanoudt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #40613|0 |1 is obsolete| | --- Comment #6 from Nicholas van Oudtshoorn <vanoudt@gmail.com> --- Created attachment 40852 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=40852&action=edit Bug 14407 - Allow web-based self checkout to be limited by IP addresses/ranges Allow web-based self-checkout to be limited to only certain IPs or IP ranges 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". If this patch is acceptable, would another to adjust opac-search to use Net::CIDR be a nice "tidying to one standard" patch? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14407 --- Comment #7 from Robin Sheat <robin@catalyst.net.nz> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14407 Nicholas van Oudtshoorn <vanoudt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #40852|0 |1 is obsolete| | --- Comment #8 from Nicholas van Oudtshoorn <vanoudt@gmail.com> --- Created attachment 40854 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=40854&action=edit Bug 14407 - Allow web-based self checkout to be limited by IP addresses/ranges (In reply to Robin in Comment #7)
Brilliant, Net::CIDR has been in Debian since the dawn of time, so that makes me happier :) :-) Glad to have brought a sigh of relief!
You've missed updating the use lines in sco-patron-image.pl though. D'oh! Fixed that now
Also don't forget to update C4/Installer/PerlDependencies.pm (keep an eye on the version version numbers being correct, wheezy has 0.15.) Ah! Hadn't thought about that at all. Should be done now.
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. 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.)
-- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14407 --- Comment #9 from Robin Sheat <robin@catalyst.net.nz> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14407 Nicholas van Oudtshoorn <vanoudt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff --- Comment #10 from Nicholas van Oudtshoorn <vanoudt@gmail.com> --- Thanks Robin! In that case, I'll change it to "needs signoff"! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14407 Zeno Tajoli <z.tajoli@cineca.it> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Medium patch -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14407 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |frederic@tamil.fr --- Comment #11 from Frédéric Demians <frederic@tamil.fr> --- 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()? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14407 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply CC| |veron@veron.ch --- Comment #12 from Marc Véron <veron@veron.ch> --- Sorry, patch does not apply: Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules Merge conflict in C4/Installer/PerlDependencies.pm -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14407 --- Comment #13 from Frédéric Demians <frederic@tamil.fr> --- I'm interested in this functionality. Without response from Nicholas van Oudtshoorn, I will refactor his patch. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14407 Nicholas van Oudtshoorn <vanoudt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #40854|0 |1 is obsolete| | --- Comment #14 from Nicholas van Oudtshoorn <vanoudt@gmail.com> --- Created attachment 45894 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=45894&action=edit Limit web-based self-checkout to specific IP addresses Allow web-based self-checkout to be limited to only certain IPs or IP ranges. Adds a function to C4::Auth called in_ipset that checks if the current client IP is in a provided IP set, as suggested in comment #11. (This function will be useful for other IP-based restrictions) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14407 Nicholas van Oudtshoorn <vanoudt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14407 Nicholas van Oudtshoorn <vanoudt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #45894|0 |1 is obsolete| | --- Comment #15 from Nicholas van Oudtshoorn <vanoudt@gmail.com> --- Created attachment 45898 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=45898&action=edit Limit web-based self-checkout to specific IP addresses Minor change to the last patch, that simply also removes a no longer needed use Net::CIDR -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14407 Nicholas van Oudtshoorn <vanoudt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #45898|0 |1 is obsolete| | --- Comment #16 from Nicholas van Oudtshoorn <vanoudt@gmail.com> --- Created attachment 45899 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=45899&action=edit Limit web-based self-checkout to specific IP addresses Minor change to the last patch, that simply also removes a no longer needed use Net::CIDR. Also removes the whitespace error :-) -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org