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 support checking same security matchers #16186

Merged
merged 4 commits into from
Dec 19, 2024

Conversation

franticticktick
Copy link
Contributor

Closes gh-15982

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 28, 2024
@franticticktick franticticktick force-pushed the gh-15982 branch 2 times, most recently from c81f19e to 7fb98ba Compare November 28, 2024 19:28
Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

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

Thanks, @franticticktick! I've left my feedback inline.

Also, if you want to add #16213 to this PR as a separate commit, I think that makes sense.

@jzheaux jzheaux added the type: enhancement A general enhancement label Dec 5, 2024
@jzheaux jzheaux self-assigned this Dec 5, 2024
@jzheaux jzheaux added this to the 6.5.x milestone Dec 5, 2024
@jzheaux
Copy link
Contributor

jzheaux commented Dec 10, 2024

@franticticktick, given #16249, I'd like to change my recommendation as that ticket would remove the notion of "after-any" unreachability. Thus, the idea of having some kind of validation reporter may be more brittle than I had first considered (since one of the handlers could be immediately deprecated).

Instead, I think a new implementation of FilterChainValidator would be better. In this way, the XML validation remains unaffected. I think WebSecurityFilterChainValidator would be a good name. Do you have any concerns about that approach?

@jzheaux jzheaux added in: web An issue in web modules (web, webmvc) and removed status: waiting-for-triage An issue we've not yet triaged labels Dec 10, 2024
@franticticktick
Copy link
Contributor Author

@jzheaux maybe it makes sense to open a new ticket? In this issue, I can fix bugs in DefaultFilterChainValidator, and add equals and hashCode to OrRequestMatcher in a separate commit. In general, i can start working on the WebSecurityFilterChainValidator in the 16213.

@jzheaux
Copy link
Contributor

jzheaux commented Dec 14, 2024

Good points, @franticticktick.

My thought is that the improvements to DefaultFilterChainValidator are more a polish and doesn't necessarily require a ticket. So perhaps that simply moves to a separate commit in this PR.

Then, there should be another commit that solves #15982 which includes improving the matchers as well as adding the check to WebSecurityFilterChainValidator. To that end, I've pushed a commit to add WebSecurityFilterChainValidator already so this commit will only be focused on adding the feature.

How does that sound?

@franticticktick franticticktick force-pushed the gh-15982 branch 4 times, most recently from 4967fe5 to 5f1ee19 Compare December 16, 2024 10:33
@franticticktick
Copy link
Contributor Author

@jzheaux thanks for the recommendations, I added tests for WebSecurityFilterChainValidator and a new exception type UnreachableFilterChainException.

Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

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

Thanks for the update, @franticticktick! I've left feedback inline.

Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

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

Thanks for the update, @franticticktick, it looks good.

My apologies there is one item I forgot, please see my inline comment for details.

@franticticktick
Copy link
Contributor Author

@jzheaux thanks for your feedback, everything is done.

@franticticktick
Copy link
Contributor Author

@jzheaux could you please review the third commit? It solves 16213

@jzheaux jzheaux removed this from the 6.5.x milestone Dec 19, 2024
@jzheaux jzheaux added this to the 6.5.0-M1 milestone Dec 19, 2024
@jzheaux jzheaux merged commit 9ae432f into spring-projects:main Dec 19, 2024
6 checks passed
@jzheaux
Copy link
Contributor

jzheaux commented Dec 19, 2024

Thanks, @franticticktick. Another very helpful PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Fail when several filter chains have the same securityMatcher
3 participants