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

[SHIRO-764] Add IpFilter for restricting access IP ranges #219

Merged
merged 1 commit into from May 6, 2020

Conversation

mookkiah
Copy link

Resolved merge conflict from original PR #57 as per @fpapon recommendation.

@fpapon
Copy link
Member

fpapon commented Apr 30, 2020

@mookkiah thanks for the PR!
Can you squash the commits please?

@fpapon fpapon changed the title Add IpFilter for restricting access IP ranges [SHIRO-764] Add IpFilter for restricting access IP ranges Apr 30, 2020
@mookkiah
Copy link
Author

Don't we loose the original contributor (@rswheeldon) name when we squash? I don't see option in the github UI to squash. Do you want me to recreate another PR by squashing all commits ?

@fpapon
Copy link
Member

fpapon commented Apr 30, 2020

You can choose the commits that you want to squash, not all.
There is no Github button but you can do it in command line like:
git rebase -i HEAD~7
The number is the commits history count.
I can deal with it if you are not familiar with this ;)

@fpapon
Copy link
Member

fpapon commented Apr 30, 2020

Don't we loose the original contributor (@rswheeldon) name when we squash? I don't see option in the github UI to squash. Do you want me to recreate another PR by squashing all commits ?

No need to recreate a PR

@mookkiah
Copy link
Author

Don't we loose the original contributor (@rswheeldon) name when we squash? I don't see option in the github UI to squash. Do you want me to recreate another PR by squashing all commits ?

No need to recreate a PR

Squashed. Learned to squash via command line. So far addicted to GitLab's checkbox to do the same.

* /another/path/** = localLan
* </pre>
*
* @since 1.4
Copy link
Author

Choose a reason for hiding this comment

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

since version needs change based on the decision what version this change is going to be merged. @fpapon @bmhm

Copy link
Member

Choose a reason for hiding this comment

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

you can set to 1.5, we will have a maintance branch for 1.5.x.

Copy link
Author

Choose a reason for hiding this comment

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

version set to 1.5.

@mookkiah
Copy link
Author

mookkiah commented May 4, 2020

@fpapon Is there any challenge in accepting this pull request. Or any action needed from my end?

Copy link
Member

@fpapon fpapon left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks!

* IPv4 address will never match a request which returns an IPv6 address, and vice-versa.
*
* @see <a href="https://github.com/spring-projects/spring-security/blob/master/web/src/main/java/org/springframework/security/web/util/matcher/IpAddressMatcher.java">Original Spring Security version</a>
* @since 1.5
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be set to the next version

Copy link
Member

Choose a reason for hiding this comment

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

@bdemers I was thinking to release it in the 1.5.4 too. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I have no objection to another 1.x release. Though my pref for this change would be a 1.6 (as it is a new feature)

We can also, just mark this as some sort of placeholder like NEXT_VERSION and then update it before the release (if we are undecided on the exact version)

Copy link
Member

Choose a reason for hiding this comment

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

@bdemers ok as it's a new feature. I would prefer to move on 2.0 to keep focus on it.

Copy link
Member

@bdemers bdemers left a comment

Choose a reason for hiding this comment

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

nit: the @SInCE tags need to be updated before merging

(likely to 2.0)

Other than that, it looks good to me!

Thanks!

Copy link
Member

@fpapon fpapon left a comment

Choose a reason for hiding this comment

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

@mookkiah Can you change the @SInCE tag to 2.0?
sorry for the updates

…om without (or outside) specific IP ranges.

Add IpAddressMatcher taken from Spring Security used for range tests

IpFilter fixes based on code review comments

Add ip to DefaultFilter
@mookkiah
Copy link
Author

mookkiah commented May 6, 2020

@fpapon @bdemers Changed the since version to 2.0

@mookkiah mookkiah requested review from bdemers and fpapon May 6, 2020 04:06
@fpapon fpapon merged commit a92a79c into apache:master May 6, 2020
@fpapon
Copy link
Member

fpapon commented Nov 15, 2020

请问此项更新预计在多久发布,hostfilter为何久久没有完善?

@721806280 can you write comments in english please?

@721806280
Copy link

@fpapon Ipfilter When is this update expected to be released? Why hasn't Hostfilter been perfected for a long time?

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.

None yet

4 participants