[Bug 43258] New: Add a centralized IP range matching helper
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43258 Bug ID: 43258 Summary: Add a centralized IP range matching helper Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: tomascohen@gmail.com QA Contact: testopia@bugs.koha-community.org Depends on: 20846 Target Milestone: --- Several OPAC features check whether the client IP matches a configured range (OpacSuppressionByIPRange, RestrictedPageLocalIPs, SelfCheckAllowByIPRanges, etc). Each repeats the same inline pattern: my $ip_address = $ENV{'REMOTE_ADDR'}; my $in_range = ( $ip_address =~ /^$ip_range/ ); This should be centralized into a helper method (e.g. on C4::Context or a utility class) so callers can simply ask 'does the current request match this range?' without reimplementing the logic. A centralized helper would also be the right place to: - Handle edge cases (undef REMOTE_ADDR, empty range) - Document that the matching is regex-based, not CIDR - Eventually support CIDR notation if needed Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20846 [Bug 20846] Suppressed records still appear in public lists -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43258 --- Comment #1 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Analysis of IP-based restriction patterns in Koha: Current state - three different matching strategies coexist: 1. C4::Auth::in_iprange (used by SelfCheckAllowByIPRanges) - Proper CIDR matching via Net::CIDR - Reads REMOTE_ADDR internally - Space-separated ranges - Already exported, used by opac/sco/*.pl 2. Regex prefix match (used by OpacSuppressionByIPRange, RestrictedPageLocalIPs) - Simple: $ip =~ /^$range/ - Not CIDR-aware, dot matches any char - Repeated inline in opac-search.pl, opac-shelves.pl, C4::Output, opac-restrictedpage.pl 3. Net::Netmask (used by ILS-DI AuthorizedIPs) - $netmask->match($ENV{REMOTE_ADDR}) - Only in opac/ilsdi.pl Places where IP restriction logic exists: - opac/sco/sco-main.pl (SelfCheckAllowByIPRanges via in_iprange) - opac/sco/help.pl (same) - opac/sco/printslip.pl (same) - opac/sco/sco-patron-image.pl (same) - opac/opac-search.pl (OpacSuppressionByIPRange, regex prefix) - opac/opac-shelves.pl (OpacSuppressionByIPRange, regex prefix, x2) - C4/Output.pm redirect_if_opac_suppressed (OpacSuppressionByIPRange, regex prefix) - opac/opac-restrictedpage.pl (RestrictedPageLocalIPs, regex prefix) - opac/ilsdi.pl (ILS-DI:AuthorizedIPs, Net::Netmask) - C4/Auth.pm session IP validation (direct comparison) - Koha/Template/Plugin/Branches.pm (REMOTE_ADDR for branch detection) Open questions for design: - Should this live in Koha::Policy::Access::IPRange (following the Policy pattern for resolving configuration into a decision)? - Or is it a lower-level utility (just 'does this IP match this range') that Policy classes and controllers both consume? - Should we unify on CIDR (Net::CIDR, which in_iprange already uses) and deprecate the regex prefix approach? - The regex approach is simpler for users to configure ('172.18.' vs '172.18.0.0/16') - do we keep both syntaxes? - in_iprange currently lives in C4::Auth which is not the right home for a general utility Note: in_iprange already does what we need. The main work is: 1. Relocate it (Koha::Network::Utils? Koha::IP? Koha::Policy::IPAccess?) 2. Decide if the regex prefix syntax should be supported alongside CIDR 3. Replace the inline regex checks with the centralized helper 4. Ensure test coverage for edge cases (undef REMOTE_ADDR, empty range, IPv6) -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43258 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | CC| |tomascohen@gmail.com -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43258 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |43261 Blocks| |43262 Blocks| |43263 CC| |kyle@bywatersolutions.com Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43261 [Bug 43261] Public biblio API ignores OpacSuppressionByIPRange https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43262 [Bug 43262] Public biblio items API does not check OpacSuppression https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43263 [Bug 43263] Public biblio ratings API does not check OpacSuppression -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43258 --- Comment #2 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- It looks like bug 28657 is using in_iprange() - maybe it already exists? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43258 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=28657 --- Comment #3 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (In reply to Katrin Fischer from comment #2)
It looks like bug 28657 is using in_iprange() - maybe it already exists?
Right... your second comment already mentions it. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43258 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff Patch complexity|--- |Trivial patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43258 --- Comment #4 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 203420 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203420&action=edit Bug 43258: Add Koha::IP utility class Introduce Koha::IP with a single class method is_ip_in_range() that centralizes IP address matching logic. Currently, at least four system preferences use IP range matching (OpacSuppressionByIPRange, RestrictedPageLocalIPs, SelfCheckAllowByIPRanges, ILS-DI:AuthorizedIPs) each reimplementing the check inline with inconsistent approaches (regex prefix, Net::CIDR, Net::Netmask). Koha::IP->is_ip_in_range({ ip => $ip, range => $range }) supports all existing syntax styles: - Prefix strings: '172.18.' (legacy, used by OpacSuppressionByIPRange) - CIDR notation: '192.168.0.0/16' - Explicit ranges: '10.0.0.1-10.0.0.255' - Space-separated lists combining any of the above This is a proof-of-concept class intended as the canonical low-level matcher. Controllers read their own syspref and pass the value here. A future Mojo helper plugin can wrap this for REST API controllers, sourcing the IP from $c->tx->remote_address instead of $ENV{REMOTE_ADDR}. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/Koha/IP.t => SUCCESS: All tests pass 3. Tests cover: - Edge cases (undef/empty IP or range) - Prefix matching (anchored, no regex injection) - CIDR matching via Net::CIDR - Explicit range notation - Space-separated mixed lists 4. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43258 --- Comment #5 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 203421 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203421&action=edit Bug 43258: (follow-up) Use Koha::IP in opac-shelves.pl Replace the inline regex IP range checks in opac-shelves.pl with Koha::IP->is_ip_in_range(). Demonstrates the intended usage pattern. Test plan: 1. Apply patch 2. Enable OpacSuppression 3. Find your IP: ktd_proxy --show-ip 4. Set OpacSuppressionByIPRange to match it (e.g. '172.18.') 5. View a public list containing suppressed records in the OPAC => SUCCESS: All records (including suppressed) are visible 6. Set OpacSuppressionByIPRange to a non-matching range (e.g. '10.99.') 7. Reload the list => SUCCESS: Suppressed records are hidden, count is correct 8. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43258 --- Comment #6 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 203422 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203422&action=edit Bug 43258: (follow-up) Add Koha::REST::Plugin::Restrictions Add a Mojolicious plugin that provides the ip_in_range helper for REST API controllers. It wraps Koha::IP->is_ip_in_range(), sourcing the client IP from $c->tx->remote_address (the Mojo equivalent of $ENV{REMOTE_ADDR}). Usage in controllers: if ( $c->ip_in_range( C4::Context->preference('OpacSuppressionByIPRange') ) ) { # client is in the exception range } The plugin is registered in Koha::REST::V1 startup alongside the existing plugins (Pagination, Query, Objects, Exceptions). Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/Koha/REST/Plugin/Restrictions.t => SUCCESS: All tests pass 3. Tests verify the helper correctly delegates to Koha::IP using the request's remote address (127.0.0.1 from Test::Mojo) 4. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43258 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43258 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #203420|0 |1 is obsolete| | Attachment #203421|0 |1 is obsolete| | Attachment #203422|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43258 --- Comment #7 from David Nind <david@davidnind.com> --- Created attachment 203425 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203425&action=edit Bug 43258: Add Koha::IP utility class Introduce Koha::IP with a single class method is_ip_in_range() that centralizes IP address matching logic. Currently, at least four system preferences use IP range matching (OpacSuppressionByIPRange, RestrictedPageLocalIPs, SelfCheckAllowByIPRanges, ILS-DI:AuthorizedIPs) each reimplementing the check inline with inconsistent approaches (regex prefix, Net::CIDR, Net::Netmask). Koha::IP->is_ip_in_range({ ip => $ip, range => $range }) supports all existing syntax styles: - Prefix strings: '172.18.' (legacy, used by OpacSuppressionByIPRange) - CIDR notation: '192.168.0.0/16' - Explicit ranges: '10.0.0.1-10.0.0.255' - Space-separated lists combining any of the above This is a proof-of-concept class intended as the canonical low-level matcher. Controllers read their own syspref and pass the value here. A future Mojo helper plugin can wrap this for REST API controllers, sourcing the IP from $c->tx->remote_address instead of $ENV{REMOTE_ADDR}. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/Koha/IP.t => SUCCESS: All tests pass 3. Tests cover: - Edge cases (undef/empty IP or range) - Prefix matching (anchored, no regex injection) - CIDR matching via Net::CIDR - Explicit range notation - Space-separated mixed lists 4. Sign off :-D Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43258 --- Comment #8 from David Nind <david@davidnind.com> --- Created attachment 203426 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203426&action=edit Bug 43258: (follow-up) Use Koha::IP in opac-shelves.pl Replace the inline regex IP range checks in opac-shelves.pl with Koha::IP->is_ip_in_range(). Demonstrates the intended usage pattern. Test plan: 1. Apply patch 2. Enable OpacSuppression 3. Find your IP: ktd_proxy --show-ip 4. Set OpacSuppressionByIPRange to match it (e.g. '172.18.') 5. View a public list containing suppressed records in the OPAC => SUCCESS: All records (including suppressed) are visible 6. Set OpacSuppressionByIPRange to a non-matching range (e.g. '10.99.') 7. Reload the list => SUCCESS: Suppressed records are hidden, count is correct 8. Sign off :-D Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43258 --- Comment #9 from David Nind <david@davidnind.com> --- Created attachment 203427 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203427&action=edit Bug 43258: (follow-up) Add Koha::REST::Plugin::Restrictions Add a Mojolicious plugin that provides the ip_in_range helper for REST API controllers. It wraps Koha::IP->is_ip_in_range(), sourcing the client IP from $c->tx->remote_address (the Mojo equivalent of $ENV{REMOTE_ADDR}). Usage in controllers: if ( $c->ip_in_range( C4::Context->preference('OpacSuppressionByIPRange') ) ) { # client is in the exception range } The plugin is registered in Koha::REST::V1 startup alongside the existing plugins (Pagination, Query, Objects, Exceptions). Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/Koha/REST/Plugin/Restrictions.t => SUCCESS: All tests pass 3. Tests verify the helper correctly delegates to Koha::IP using the request's remote address (127.0.0.1 from Test::Mojo) 4. Sign off :-D Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43258 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com Keywords| |release-notes-needed -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43258 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the| |This development refactors release notes| |the current code so all | |places in which IP | |addresses are checked share | |the same logic and | |behavior. Untested | |controller code for this is | |replaced by a fully tested | |library. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org