Skip to content

Add DestinationPathPatternMessageMatcher #16635

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

Merged

Conversation

pat-mccusker
Copy link
Contributor

Closes gh-16500

I wasn't sure that this configuration was safe to remove so I wanted to point it out. In WebSocketMessageBrokerSecurityConfigurationTests an AntPathMatcher is configured with a '.' path separator, and this test will fail if the mentioned config is simply removed. If consumers of spring-security are configuring a PathMatcher in a SimpAnnotationMethodMessageHandler instance, then this matching would cease to function correctly as far as I can tell.

This led me to delegating the destination pattern matching to one or another PathPatternRouteMatchers depending on which path separator is configured by the MessageMatcherDelegatingAuthorizationManager.Builder, but I've left the original builder configuration in place for the moment.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 23, 2025
@pat-mccusker pat-mccusker force-pushed the gh-16500-deprecate-path_matcher branch from de8c0ce to 31b962b Compare February 25, 2025 16:48
@pat-mccusker
Copy link
Contributor Author

pat-mccusker commented Feb 25, 2025

Hi @jzheaux, not sure if there are notifications on Draft PRs but I wanted to get your thoughts on how things looked in here so far.

@jzheaux jzheaux self-assigned this Feb 27, 2025
@jzheaux jzheaux added in: messaging An issue in spring-security-messaging type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 27, 2025
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 draft, @pat-mccusker, this is a great start.

As far as your question about the prototype bean method, we should have an opt-in strategy that says that an application wants to start using DestinationPathPatternMessageMatcher instead of PathMatcher.

I think you can follow the pattern in PathPatternRequestMatcherBuilderFactoryBean. This is a factory bean that publishes a PathPatternRequestMatcher.Builder instances that applications and the DSL can use to build request matchers.

If you introduced DestinationPathPatternMessageMatcherBuilderFactoryBean, then you could do the same here, e.g. "if DestinationPathPatternMessageMatcher.Builder bean exists, then I use that to construct request matchers instead of working with path matcher"

@pat-mccusker pat-mccusker force-pushed the gh-16500-deprecate-path_matcher branch from 31b962b to d32d0c4 Compare March 2, 2025 17:33
@pat-mccusker
Copy link
Contributor Author

pat-mccusker commented Mar 2, 2025

I've addressed some of the more straightforward items involving the PathPatternMessageMatcher itself and will keep working on the rest.

@pat-mccusker pat-mccusker force-pushed the gh-16500-deprecate-path_matcher branch 2 times, most recently from 48183da to 542ee2b Compare March 7, 2025 18:57
@pat-mccusker pat-mccusker force-pushed the gh-16500-deprecate-path_matcher branch 2 times, most recently from 1c68492 to 1e46171 Compare March 11, 2025 21:10
@pat-mccusker pat-mccusker requested a review from jzheaux March 11, 2025 21:12
@pat-mccusker pat-mccusker force-pushed the gh-16500-deprecate-path_matcher branch from 1e46171 to f58b1ff Compare March 16, 2025 19:28
@pat-mccusker pat-mccusker marked this pull request as ready for review March 16, 2025 19:33
@pat-mccusker pat-mccusker force-pushed the gh-16500-deprecate-path_matcher branch 4 times, most recently from c3fc150 to 8e391f5 Compare March 17, 2025 19:33
@jzheaux jzheaux force-pushed the gh-16500-deprecate-path_matcher branch 3 times, most recently from f530f22 to aa56d0a Compare March 18, 2025 21:54
pat-mccusker and others added 3 commits March 18, 2025 15:56
Closes spring-projectsgh-16766

Signed-off-by: Pat McCusker <patmccusker14@gmail.com>
Closes spring-projectsgh-16500

Signed-off-by: Pat McCusker <patmccusker14@gmail.com>
Issue spring-projectsgh-16500

Signed-off-by: Josh Cummings <3627351+jzheaux@users.noreply.github.com>
@jzheaux jzheaux force-pushed the gh-16500-deprecate-path_matcher branch from aa56d0a to 949d1cc Compare March 18, 2025 21:56
@jzheaux jzheaux added this to the 6.5.0-RC1 milestone Mar 18, 2025
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, @pat-mccusker! I added a polish commit that brings the logic and documentation further into alignment with the request matcher changes.

Also, some of the logic in the request matcher changes was there to support the additional complexity of the configurable Spring MVC matcher. Because that isn't present here, I was able to remove some of the logic.

@jzheaux jzheaux merged commit 44d5539 into spring-projects:main Mar 19, 2025
6 checks passed
@jzheaux
Copy link
Contributor

jzheaux commented Mar 19, 2025

Thanks, @pat-mccusker, for all your work here. This is now merged into main.

@pat-mccusker
Copy link
Contributor Author

Awesome, thanks @jzheaux for all the direction and prior examples!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: messaging An issue in spring-security-messaging type: enhancement A general enhancement
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Deprecate usages of PathMatcher in Web Socket support
3 participants