Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add IpFilter for restricting access IP ranges #57

Closed
wants to merge 3 commits into from

Conversation

rswheeldon
Copy link

Add IpFilter for restricting access to resources from those coming from without (or outside) specific IP ranges.

Add IpAddressMatcher taken from Spring Security used for range tests

…om without (or outside) specific IP ranges.

Add IpAddressMatcher taken from Spring Security used for range tests
* Both IPv6 and IPv4 addresses are supported, but a matcher which is configured with an
* IPv4 address will never match a request which returns an IPv6 address, and vice-versa.
*
* @author Luke Taylor originally written for Spring Security
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@author tags should be removed, but a link to the original source would be great.

You can also update the NOTICE if needed. The current wording my be sufficient, but I'll leave this comment here, incase anyone else has thoughts on this.

* address ranges and / or not from with a specific (denied) set.
* <p/>
* Example config:
* <pre>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Old Javadoc
The [filters] section is deprecated. but you can replace this with [main]

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also changing in PortFilter - which is where I cribbed it from.

* /another/path/** = localLan
* </pre>
*
* @since 1.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be 1.4


import java.util.Collection;

public interface IpSource {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add @since tag

import org.junit.Test;

/**
* @author Modified by Richard Wheeldon from an original by Luke Taylor for Spring Security
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment about @author above

* @author Modified by Richard Wheeldon from an original by Luke Taylor for Spring Security
* @since 1.4.0
*/
public class IpAddressMatcherTests {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for including the tests as well!!

@bdemers
Copy link
Member

bdemers commented Feb 1, 2017

This should probably be added to DefaultFilter as well. Possibly using the name ipRange (not sure of that name though, ips seems too short, ipFilter doesn't match the current naming scheme, other ideas welcome)

@bdemers
Copy link
Member

bdemers commented Feb 1, 2017

@rswheeldon Thanks for working on this! I added a few minor things, nothing major. You should also sign a Apache CLA.

@rswheeldon
Copy link
Author

CLA was signed years ago when I pushed a load of stuff to Apache FOP. It should still be valid. If not, I'll sign a new one.

@bdemers
Copy link
Member

bdemers commented Feb 1, 2017

Cool, I only took a quick look, I'll look again

@rswheeldon
Copy link
Author

rswheeldon commented Feb 2, 2017

Updated with fixes for all the above.

@bdemers
Copy link
Member

bdemers commented Feb 3, 2017

@rswheeldon
I found the CLA, I must have fat fingered the search last time.

Any thoughts on the DefaultFilter naming ?

@rswheeldon
Copy link
Author

ip - it's only one character shorter than ssl. rest, port and user aren't exactly verbose either. If you agree, I'll add it.

@bdemers
Copy link
Member

bdemers commented Feb 3, 2017

ip works for me

@rswheeldon
Copy link
Author

Done. I don't really understand the Guice stuff though so I've done nothing more on that than make the unit tests pass.

@rswheeldon
Copy link
Author

What else is needed to get this into trunk? There are no changes I'm intending on making unless there's something that's not been addressed.

@bdemers
Copy link
Member

bdemers commented May 2, 2017

This looks good to me 👍

@col-panic
Copy link

Whats the status on this? Currently looking for this kind of functionality?!

@bdemers
Copy link
Member

bdemers commented Mar 16, 2018

@col-panic give this PR a shot and let us know what you think!

@mookkiah
Copy link

Hello @col-panic @bdemers - We are looking towards to have this IP filter option in our application design. But seeing this PR left open concerns me. Is there any reason or alternative.
If it is just the merge conflict, I can contribute by resolving it. Please let me know

@fpapon
Copy link
Member

fpapon commented Apr 30, 2020

@mookkiah feel free to resolve the conflict, we will merge it!

@col-panic
Copy link

I switched my implementation to be behind an nginx that acts as reverse proxy. This allows me to separate this concerns from the filter configuration which is now done in nginx. Due to this I did not further spend time on it. sry.

mookkiah added a commit to mookkiah/shiro that referenced this pull request Apr 30, 2020
@mookkiah
Copy link

@fpapon Created new PR #219 after resolving merge conflict.
@col-panic I agree that we can block IP using infrastructure solution like nginx. That is an option in our design as well. Being an Apache Shiro user, I had expectation to have this solution (at least as a configurable option).
Hope this is useful for someone who wants to use simple option.

@fpapon
Copy link
Member

fpapon commented Apr 30, 2020

@mookkiah thanks! I close this one.

@fpapon fpapon closed this Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants