Add DestinationPathPatternMessageMatcher #16635

pat-mccusker · 2025-02-23T17:30:59Z

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.

Closes spring-projectsgh-16500 Signed-off-by: Pat McCusker <[email protected]>

pat-mccusker · 2025-02-25T17:05:43Z

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

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"

jzheaux · 2025-02-24T21:38:11Z

...rg/springframework/security/messaging/util/matcher/DestinationPathPatternMessageMatcher.java

+	 * {@link org.springframework.http.server.PathContainer.Options#MESSAGE_ROUTE}
+	 * separator
+	 */
+	public static Builder messageRoute() {


Instead of messageRoute(), let's expose the PathPatternParser that will be used. This will allow the application to configure PathOptions as well as anything else needed.

jzheaux · 2025-02-27T19:53:21Z

...mework/security/messaging/access/intercept/MessageMatcherDelegatingAuthorizationManager.java

+		 * {@code Constraint} will be applicable
+		 * @since 6.5
+		 */
+		public Builder.Constraint destinationPathPatterns(String... patterns) {


If there is another way to opt-in, I'd prefer to leave the methods that take a String the same. The reason is that it still describes what the application is doing: matching a simp destination.

jzheaux · 2025-02-27T19:55:10Z

...mework/security/messaging/access/intercept/MessageMatcherDelegatingAuthorizationManager.java

+		 * configured in
+		 * {@link org.springframework.http.server.PathContainer.Options#MESSAGE_ROUTE}
+		 */
+		public Builder messageRouteSeparator() {


This is probably too specific for the DSL. The application in this case should constructor their own message matcher instance.

jzheaux · 2025-02-27T19:57:25Z

...mework/security/messaging/access/intercept/MessageMatcherDelegatingAuthorizationManager.java

@@ -224,7 +324,9 @@ public Builder simpDestPathMatcher(PathMatcher pathMatcher) {
 		 * computation or lookup of the {@link PathMatcher}.
 		 * @param pathMatcher the {@link PathMatcher} to use. Cannot be null.
 		 * @return the {@link Builder} for further customization.
+		 * @deprecated use {@link #messageRouteSeparator()} to alter the path separator


Instead, let's begin with directing them to use #matchers to construct their own matcher.

jzheaux · 2025-02-27T20:00:50Z

...mework/security/messaging/access/intercept/MessageMatcherDelegatingAuthorizationManager.java

@@ -412,6 +513,37 @@ Map<String, String> extractPathVariables(Message<?> message) {

 		}

+		private static final class LazySimpDestinationPatternMessageMatcher implements MessageMatcher<Object> {


The existing deferral work is there to address an eager instantiation issue caused by trying to resolve the Spring Websocket PathMatcher instance too early. When we aren't using PathMatcher, then this lazy-loading is likely unnecessary.

jzheaux · 2025-02-27T20:28:50Z

...k/security/messaging/access/intercept/MessageMatcherDelegatingAuthorizationManagerTests.java

@@ -43,30 +43,30 @@ public final class MessageMatcherDelegatingAuthorizationManagerTests {
 	void checkWhenPermitAllThenPermits() {
 		AuthorizationManager<Message<?>> authorizationManager = builder().anyMessage().permitAll().build();
 		Message<?> message = new GenericMessage<>(new Object());
-		assertThat(authorizationManager.check(mock(Supplier.class), message).isGranted()).isTrue();


Please leave these as-is. Even though check is deprecated, we want to make sure it keeps working until we are ready to remove it.

jzheaux · 2025-02-27T20:30:54Z

...k/security/messaging/access/intercept/MessageMatcherDelegatingAuthorizationManagerTests.java

+	@Test
+	void checkWhenMessageTypeAndPathPatternMatches() {
+		AuthorizationManager<Message<?>> authorizationManager = builder()
+			.simpTypeMessageDestinationPatterns("/destination")


Please follow the following convention:

.matcher(pathOne).rule() .matcher(pathTwo).rule() // ...

So, in this case, it would look something like:

.simpTypeMessageDestinationPatterns("/destination").permitAll() .simpTypeSubscribeDestinationPatterns("/destination").denyAll() // ...

jzheaux · 2025-02-27T20:32:23Z

.../security/config/annotation/web/socket/WebSocketMessageBrokerSecurityConfigurationTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2024 the original author or authors.
+ * Copyright 2002-2025 the original author or authors.


Since there are no other changes here, please go ahead and revert the copyright year so that this file is left out of the PR. Please do this for any other files like this one.

jzheaux · 2025-02-27T20:33:13Z

...mework/security/messaging/access/intercept/MessageMatcherDelegatingAuthorizationManager.java

@@ -87,12 +89,11 @@ private MessageAuthorizationContext<?> authorizationContext(MessageMatcher<?> ma
 		if (!matcher.matches((Message) message)) {
 			return null;
 		}
-		if (matcher instanceof SimpDestinationMessageMatcher simp) {
-			return new MessageAuthorizationContext<>(message, simp.extractPathVariables(message));
+		if (matcher instanceof Builder.LazySimpDestinationMessageMatcher pathMatcher) {


If we make the change to MessageMatcher suggested in this PR, then this casting can all be removed.

jzheaux · 2025-02-27T20:46:11Z

...rg/springframework/security/messaging/util/matcher/DestinationPathPatternMessageMatcher.java

+ * @author Pat McCusker
+ * @since 6.5
+ */
+public final class DestinationPathPatternMessageMatcher implements MessageMatcher<Object> {


I think this should be called PathPatternMessageMatcher.

The reason is that it already contains more aspects of the message than the destination and may contain more in the future. So far, it doesn't seem materially different from PathPatternRequestMatcher and so we don't want to give the impression that it is by using a more specific name.

Note that otherwise, we'd want to call PathPatternRequestMatcher something more like UriPathPatternRequestMatcher. Which, at least for our purposes, doesn't add much information about what it is for.

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 23, 2025

Add DestinationPathPatternMessageMatcher

31b962b

Closes spring-projectsgh-16500 Signed-off-by: Pat McCusker <[email protected]>

pat-mccusker force-pushed the gh-16500-deprecate-path_matcher branch from de8c0ce to 31b962b Compare February 25, 2025 16:48

jzheaux self-assigned this Feb 27, 2025

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

jzheaux requested changes Feb 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DestinationPathPatternMessageMatcher #16635

Add DestinationPathPatternMessageMatcher #16635

pat-mccusker commented Feb 23, 2025

pat-mccusker commented Feb 25, 2025 •

edited

Loading

jzheaux left a comment

jzheaux Feb 24, 2025 •

edited

Loading

jzheaux Feb 27, 2025

jzheaux Feb 27, 2025

jzheaux Feb 27, 2025

jzheaux Feb 27, 2025

jzheaux Feb 27, 2025

jzheaux Feb 27, 2025

jzheaux Feb 27, 2025

jzheaux Feb 27, 2025

jzheaux Feb 27, 2025

		@@ -412,6 +513,37 @@ Map<String, String> extractPathVariables(Message<?> message) {

		}

		private static final class LazySimpDestinationPatternMessageMatcher implements MessageMatcher<Object> {

Add DestinationPathPatternMessageMatcher #16635

Are you sure you want to change the base?

Add DestinationPathPatternMessageMatcher #16635

Conversation

pat-mccusker commented Feb 23, 2025

pat-mccusker commented Feb 25, 2025 • edited Loading

jzheaux left a comment

Choose a reason for hiding this comment

jzheaux Feb 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pat-mccusker commented Feb 25, 2025 •

edited

Loading

jzheaux Feb 24, 2025 •

edited

Loading