Bug 8492 - Restrict OpacSuppression to IP addresses outside of an IP range
Summary: Restrict OpacSuppression to IP addresses outside of an IP range
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: 3.10
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Mirko Tietgen
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-23 23:31 UTC by Mirko Tietgen
Modified: 2013-05-23 06:23 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 8492 [ENH] Restrict OpacSuppression to IP adresses outside of an IP range (4.73 KB, patch)
2012-07-23 23:34 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 8492 [ENH] Restrict OpacSuppression to IP adresses outside of an IP range (4.73 KB, patch)
2012-07-27 09:43 UTC, Mirko Tietgen
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8492 [ENH] Restrict OpacSuppression to IP adresses outside of an IP range (4.84 KB, patch)
2012-07-30 16:53 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 8492 [ENH] Restrict OpacSuppression to IP adresses outside of an IP range (4.79 KB, patch)
2012-08-20 09:50 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mirko Tietgen 2012-07-23 23:31:12 UTC
This enhancement extends the OpacSuppression feature with an optional IP address range within which results are _not_ suppressed.
Comment 1 Mirko Tietgen 2012-07-23 23:34:22 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2012-07-24 07:22:40 UTC
Hi Mirko,

I am not an expert of the apache configuration but it seems it is possible to do that with the RewriteCond and RewriteRule directives.

Did you have a look at these directives ?
Comment 3 Chris Cormack 2012-07-24 07:27:14 UTC
(In reply to comment #2)
> Hi Mirko,
> 
> I am not an expert of the apache configuration but it seems it is possible
> to do that with the RewriteCond and RewriteRule directives.
> 
> Did you have a look at these directives ?

I'm not sure you could, in the same way. OpacSuppression lets you put something in 942 that makes an biblio not show in the OPAC.

This would let those still show for certain ip ranges while suppressing them for others. I'm not sure how apache could know which items to hide. Unless you made a rule for every biblio.
Comment 4 Robin Sheat 2012-07-24 08:30:07 UTC
Mirko, something to consider is that many Koha systems are behind reverse-proxies of various forms. I'm not sure exactly what Apache puts into REMOTE_ADDR, but if it doesn't consider that, it may be worth checking the request headers too (e.g. if the request is from an RFC1918 address, check to see if there's an X-Forwarded-For: header too, or something like that)
Comment 5 Mirko Tietgen 2012-07-24 11:33:17 UTC
Jonathan: what Chris says. It's about filtering some, not all results (those marked in 942$c). I don't think that can be achieved with Apache config?

Robin: good point, did a little reading on this. It seems like
- it won't work with reverse proxies like it is now
- you can spoof X-Forwarded-For headers, so using them should be optional

A check for RFC1918 addresses sounds like an interesting idea, but is it safe to assume that there would always be a private address here? I doubt that, eg. my university uses external IP addresses everywhere.
Comment 6 Mirko Tietgen 2012-07-24 12:09:00 UTC
Data::Validate::IP could do these (and more) things

is_innet_ipv4
to see if ip is in network given in syspref

is_private_ipv4
to see if ip is private

Dependencies on Debian Squeeze are available
libdata-validate-ip-perl libnet-ipv6addr-perl libnet-netmask-perl libnetwork-ipv4addr-perl

What are the rules for introducing perl modules/ dependencies?


It could be like this but would introduce a lot of sysprefs

option: use OpacSuppression yes/no
    option: match REMOTE_ADDR and network
        option: if exists, match X-Forward-For instead of REMOTE_ADDR
            option: only if REMOTE_ADDR is private IP
Comment 7 Robin Sheat 2012-07-24 12:13:59 UTC
I wouldn't expect most systems to be on real-world IP addresses if they're behind a proxy anyway, there's no point. And it's only the address of the proxy you care about. Though, thinking about it more, if you're attempting to restrict to part of an RFC1918 set, this would leave forgery open. A better idea would be to specify what your proxy IP/IP pattern is and look for the header only in that case.

X-Forwarded-For can be forged, also can (legitimately) have multiple IP addresses in it, but it is possible to be sure about what you're getting. For example, if you know you're behind a proxy you can rely on that header being there, and that the last entry is the real source IP address.
Comment 8 Mirko Tietgen 2012-07-26 14:23:55 UTC
after thinking about it a little, I propose to

- use this patch like it is to add the basic functionality without new dependencies to 3.8
- have a more advanced version using Data::Validate::IP in 3.10

Does that make sense?
Comment 9 Chris Cormack 2012-07-26 19:36:42 UTC
Makes sense to me, and a simple patch with no new dependencies, that does nothing until a syspref is checked is the perfect candidate for backport to 3.8.x.

So if that makes this through QA and into master, im happy to put it into 3.8.x and then the follow up can extend it for master only.
Comment 10 Mirko Tietgen 2012-07-27 09:43:26 UTC Comment hidden (obsolete)
Comment 11 Marc Véron 2012-07-30 16:53:14 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2012-08-20 09:50:30 UTC
Created attachment 11692 [details] [review]
Bug 8492 [ENH] Restrict OpacSuppression to IP adresses outside of an IP range

This enhancement extends the OpacSuppression feature with an optional IP address range within which results are _not_ suppressed.

To test

* turn on OpacSuppression (Administration->System preferences->Cataloging) and enter an IP address range in the OpacSuppressionByIPRange field.
* set at least one bibliographic record to suppress=1 (enter '1' in 942$n)
* fully reindex your data
* do an OPAC search that should bring up your suppressed record
* try with IP ranges that match your IP and ranges that don't

Signed-off-by: Marc Veron <veron@veron.ch>

Tested following the scenario above. Works as expected.
Comment 13 Jonathan Druart 2012-08-20 09:56:22 UTC
Last patch is a rebased patch (conflict on updatedatabase.pl).

QA Comment:

Add a new syspref OpacSuppressionByIPRange.

According to the comment 8, a followup is required for 3.10.

Marked as Passed QA.
Comment 14 Paul Poulain 2012-09-03 09:58:28 UTC
Patch pushed, but a tiny question: isn't "Restrict the suppression to IP adresses outside of the IP range" a little bit complex to understand. Wouldn't it be more understandable as "Don't suppress items in this IP range" ?

(note that I just tested my search is not broken, I haven't suppress index on my test computer)
Comment 15 Mirko Tietgen 2012-09-03 10:14:27 UTC
You are right, the wording is bad. I think it evolved from "restrict OpacSuppression to IP range" (which is technically wrong) to the monster "restrict to IP addresses outside of an IP range" which is quite hard to parse. I will try to avoid things like that in the future.
Comment 16 Paul Poulain 2012-09-03 10:29:47 UTC
(In reply to comment #15)
> You are right, the wording is bad. I think it evolved from "restrict
> OpacSuppression to IP range" (which is technically wrong) to the monster
> "restrict to IP addresses outside of an IP range" which is quite hard to
> parse. I will try to avoid things like that in the future.

Feel free to add a follow-up to this one ;-)