-
Notifications
You must be signed in to change notification settings - Fork 6k
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
Add DestinationPathPatternMessageMatcher #16635
Conversation
de8c0ce
to
31b962b
Compare
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. |
There was a problem hiding this 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"
...rg/springframework/security/messaging/util/matcher/DestinationPathPatternMessageMatcher.java
Outdated
Show resolved
Hide resolved
...mework/security/messaging/access/intercept/MessageMatcherDelegatingAuthorizationManager.java
Outdated
Show resolved
Hide resolved
...mework/security/messaging/access/intercept/MessageMatcherDelegatingAuthorizationManager.java
Outdated
Show resolved
Hide resolved
...mework/security/messaging/access/intercept/MessageMatcherDelegatingAuthorizationManager.java
Outdated
Show resolved
Hide resolved
...mework/security/messaging/access/intercept/MessageMatcherDelegatingAuthorizationManager.java
Outdated
Show resolved
Hide resolved
...k/security/messaging/access/intercept/MessageMatcherDelegatingAuthorizationManagerTests.java
Outdated
Show resolved
Hide resolved
...k/security/messaging/access/intercept/MessageMatcherDelegatingAuthorizationManagerTests.java
Outdated
Show resolved
Hide resolved
.../security/config/annotation/web/socket/WebSocketMessageBrokerSecurityConfigurationTests.java
Outdated
Show resolved
Hide resolved
...mework/security/messaging/access/intercept/MessageMatcherDelegatingAuthorizationManager.java
Outdated
Show resolved
Hide resolved
...rg/springframework/security/messaging/util/matcher/DestinationPathPatternMessageMatcher.java
Outdated
Show resolved
Hide resolved
31b962b
to
d32d0c4
Compare
I've addressed some of the more straightforward items involving the |
48183da
to
542ee2b
Compare
1c68492
to
1e46171
Compare
...ngframework/security/messaging/util/matcher/PathPatternMessageMatcherBuilderFactoryBean.java
Outdated
Show resolved
Hide resolved
1e46171
to
f58b1ff
Compare
c3fc150
to
8e391f5
Compare
f530f22
to
aa56d0a
Compare
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>
aa56d0a
to
949d1cc
Compare
There was a problem hiding this 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.
Thanks, @pat-mccusker, for all your work here. This is now merged into |
Awesome, thanks @jzheaux for all the direction and prior examples! |
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 aPathMatcher
in aSimpAnnotationMethodMessageHandler
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
PathPatternRouteMatcher
s depending on which path separator is configured by theMessageMatcherDelegatingAuthorizationManager.Builder
, but I've left the original builder configuration in place for the moment.