PathPatternMessageMatcher Polish

Issue gh-16500

Signed-off-by: Josh Cummings <[email protected]>

Author: jzheaux

messaging/src/main/java/org/springframework/security/messaging/util/matcher/PathPatternMessageMatcherBuilderFactoryBean.java renamed to config/src/main/java/org/springframework/security/config/web/messaging/PathPatternMessageMatcherBuilderFactoryBean.java

@@ -14,36 +14,26 @@
  * limitations under the License.
  */
 
-package org.springframework.security.messaging.util.matcher;
+package org.springframework.security.config.web.messaging;
 
 import org.springframework.beans.factory.FactoryBean;
-import org.springframework.web.util.pattern.PathPatternParser;
+import org.springframework.security.messaging.access.intercept.MessageMatcherDelegatingAuthorizationManager;
+import org.springframework.security.messaging.util.matcher.PathPatternMessageMatcher;
 
 /**
  * Use this factory bean to configure the {@link PathPatternMessageMatcher.Builder} bean
- * used to create request matchers in
- * {@link org.springframework.security.messaging.access.intercept.MessageMatcherDelegatingAuthorizationManager}
+ * used to create request matchers in {@link MessageMatcherDelegatingAuthorizationManager}
  * and other parts of the DSL.
  *
  * @author Pat McCusker
  * @since 6.5
  */
-public class PathPatternMessageMatcherBuilderFactoryBean implements FactoryBean<PathPatternMessageMatcher.Builder> {
-
-	private final PathPatternParser parser;
-
-	public PathPatternMessageMatcherBuilderFactoryBean() {
-		this(null);
-	}
-
-	public PathPatternMessageMatcherBuilderFactoryBean(PathPatternParser parser) {
-		this.parser = parser;
-	}
+public final class PathPatternMessageMatcherBuilderFactoryBean
+		implements FactoryBean<PathPatternMessageMatcher.Builder> {
 
 	@Override
 	public PathPatternMessageMatcher.Builder getObject() throws Exception {
-		return (this.parser != null) ? PathPatternMessageMatcher.withPathPatternParser(this.parser)
-				: PathPatternMessageMatcher.withDefaults();
+		return PathPatternMessageMatcher.withDefaults();
 	}
 
 	@Override

messaging/src/main/java/org/springframework/security/messaging/access/intercept/MessageMatcherDelegatingAuthorizationManager.java

@@ -17,9 +17,7 @@
 package org.springframework.security.messaging.access.intercept;
 
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.List;
-import java.util.Map;
 import java.util.function.Supplier;
 
 import org.apache.commons.logging.Log;
@@ -98,9 +96,6 @@ private MessageAuthorizationContext<?> authorizationContext(MessageMatcher<?> ma
 			return new MessageAuthorizationContext<>(message, matchResult.getVariables());
 		}
 
-		if (matcher instanceof Builder.LazySimpDestinationMessageMatcher pathMatcher) {
-			return new MessageAuthorizationContext<>(message, pathMatcher.extractPathVariables(message));
-		}
 		return new MessageAuthorizationContext<>(message);
 	}
 
@@ -208,7 +203,7 @@ private Builder.Constraint simpDestMatchers(SimpMessageType type, String... patt
 			List<MessageMatcher<?>> matchers = new ArrayList<>(patterns.length);
 			for (String pattern : patterns) {
 				MessageMatcher<Object> matcher = MessageMatcherFactory.usesPathPatterns()
-						? MessageMatcherFactory.matcher(pattern, type)
+						? MessageMatcherFactory.matcher(type, pattern)
 						: new LazySimpDestinationMessageMatcher(pattern, type);
 				matchers.add(matcher);
 			}
@@ -254,7 +249,9 @@ public Builder simpDestPathMatcher(Supplier<PathMatcher> pathMatcher) {
 		 */
 		public Builder.Constraint matchers(MessageMatcher<?>... matchers) {
 			List<MessageMatcher<?>> builders = new ArrayList<>(matchers.length);
-			builders.addAll(Arrays.asList(matchers));
+			for (MessageMatcher<?> matcher : matchers) {
+				builders.add(matcher);
+			}
 			return new Builder.Constraint(builders);
 		}
 
@@ -419,8 +416,9 @@ public boolean matches(Message<?> message) {
 				return this.delegate.get().matches(message);
 			}
 
-			Map<String, String> extractPathVariables(Message<?> message) {
-				return this.delegate.get().extractPathVariables(message);
+			@Override
+			public MatchResult matcher(Message<?> message) {
+				return this.delegate.get().matcher(message);
 			}
 
 		}
@@ -433,7 +431,7 @@ private static final class Entry<T> {
 
 		private final T entry;
 
-		Entry(MessageMatcher<?> requestMatcher, T entry) {
+		Entry(MessageMatcher requestMatcher, T entry) {
 			this.messageMatcher = requestMatcher;
 			this.entry = entry;
 		}

messaging/src/main/java/org/springframework/security/messaging/util/matcher/MessageMatcherFactory.java

@@ -19,6 +19,12 @@
 import org.springframework.context.ApplicationContext;
 import org.springframework.messaging.simp.SimpMessageType;
 
+/**
+ * This utility exists only to facilitate applications opting into using path patterns in
+ * the Message Security DSL. It is for internal use only.
+ *
+ * @deprecated
+ */
 @Deprecated(forRemoval = true)
 public final class MessageMatcherFactory {
 
@@ -33,11 +39,11 @@ public static boolean usesPathPatterns() {
 	}
 
 	public static MessageMatcher<?> matcher(String destination) {
-		return builder.matcher(destination);
+		return matcher(null, destination);
 	}
 
-	public static MessageMatcher<Object> matcher(String destination, SimpMessageType type) {
-		return (type != null) ? builder.matcher(destination, type) : builder.matcher(destination);
+	public static MessageMatcher<Object> matcher(SimpMessageType type, String destination) {
+		return builder.matcher(type, destination);
 	}
 
 	private MessageMatcherFactory() {

messaging/src/main/java/org/springframework/security/messaging/util/matcher/PathPatternMessageMatcher.java

@@ -19,9 +19,11 @@
 import java.util.Collections;
 
 import org.springframework.http.server.PathContainer;
+import org.springframework.lang.Nullable;
 import org.springframework.messaging.Message;
 import org.springframework.messaging.simp.SimpMessageHeaderAccessor;
 import org.springframework.messaging.simp.SimpMessageType;
+import org.springframework.security.messaging.access.intercept.MessageAuthorizationContext;
 import org.springframework.util.Assert;
 import org.springframework.web.util.pattern.PathPattern;
 import org.springframework.web.util.pattern.PathPatternParser;
@@ -40,16 +42,16 @@ public final class PathPatternMessageMatcher implements MessageMatcher<Object> {
 
 	private final PathPattern pattern;
 
-	private final PathPatternParser parser;
+	private final PathContainer.Options options;
 
 	/**
 	 * The {@link MessageMatcher} that determines if the type matches. If the type was
 	 * null, this matcher will match every Message.
 	 */
 	private MessageMatcher<Object> messageTypeMatcher = ANY_MESSAGE;
 
-	private PathPatternMessageMatcher(PathPattern pattern, PathPatternParser parser) {
-		this.parser = parser;
+	private PathPatternMessageMatcher(PathPattern pattern, PathContainer.Options options) {
+		this.options = options;
 		this.pattern = pattern;
 	}
 
@@ -78,17 +80,7 @@ void setMessageTypeMatcher(MessageMatcher<Object> messageTypeMatcher) {
 	 */
 	@Override
 	public boolean matches(Message<?> message) {
-		if (!this.messageTypeMatcher.matches(message)) {
-			return false;
-		}
-
-		String destination = getDestination(message);
-		if (destination == null) {
-			return false;
-		}
-
-		PathContainer destinationPathContainer = PathContainer.parsePath(destination, this.parser.getPathOptions());
-		return this.pattern.matches(destinationPathContainer);
+		return matcher(message).isMatch();
 	}
 
 	/**
@@ -109,7 +101,7 @@ public MatchResult matcher(Message<?> message) {
 			return MatchResult.notMatch();
 		}
 
-		PathContainer destinationPathContainer = PathContainer.parsePath(destination, this.parser.getPathOptions());
+		PathContainer destinationPathContainer = PathContainer.parsePath(destination, this.options);
 		PathPattern.PathMatchInfo pathMatchInfo = this.pattern.matchAndExtract(destinationPathContainer);
 
 		return (pathMatchInfo != null) ? MatchResult.match(pathMatchInfo.getUriVariables()) : MatchResult.notMatch();
@@ -119,33 +111,92 @@ private static String getDestination(Message<?> message) {
 		return SimpMessageHeaderAccessor.getDestination(message.getHeaders());
 	}
 
+	/**
+	 * A builder for specifying various elements of a message for the purpose of creating
+	 * a {@link PathPatternMessageMatcher}.
+	 */
 	public static class Builder {
 
 		private final PathPatternParser parser;
 
-		private MessageMatcher<Object> messageTypeMatcher = ANY_MESSAGE;
-
 		Builder(PathPatternParser parser) {
 			this.parser = parser;
 		}
 
+		/**
+		 * Match messages having this destination pattern.
+		 *
+		 * <p>
+		 * Path patterns always start with a slash and may contain placeholders. They can
+		 * also be followed by {@code /**} to signify all URIs under a given path.
+		 *
+		 * <p>
+		 * The following are valid patterns and their meaning
+		 * <ul>
+		 * <li>{@code /path} - match exactly and only `/path`</li>
+		 * <li>{@code /path/**} - match `/path` and any of its descendents</li>
+		 * <li>{@code /path/{value}/**} - match `/path/subdirectory` and any of its
+		 * descendents, capturing the value of the subdirectory in
+		 * {@link MessageAuthorizationContext#getVariables()}</li>
+		 * </ul>
+		 *
+		 * <p>
+		 * A more comprehensive list can be found at {@link PathPattern}.
+		 *
+		 * <p>
+		 * A dot-based message pattern is also supported when configuring a
+		 * {@link PathPatternParser} using
+		 * {@link PathPatternMessageMatcher#withPathPatternParser}
+		 * @param pattern the destination pattern to match
+		 * @return the {@link PathPatternMessageMatcher.Builder} for more configuration
+		 */
 		public PathPatternMessageMatcher matcher(String pattern) {
-			Assert.notNull(pattern, "Pattern must not be null");
+			return matcher(null, pattern);
+		}
+
+		/**
+		 * Match messages having this type and destination pattern.
+		 *
+		 * <p>
+		 * When the message {@code type} is null, then the matcher does not consider the
+		 * message type
+		 *
+		 * <p>
+		 * Path patterns always start with a slash and may contain placeholders. They can
+		 * also be followed by {@code /**} to signify all URIs under a given path.
+		 *
+		 * <p>
+		 * The following are valid patterns and their meaning
+		 * <ul>
+		 * <li>{@code /path} - match exactly and only `/path`</li>
+		 * <li>{@code /path/**} - match `/path` and any of its descendents</li>
+		 * <li>{@code /path/{value}/**} - match `/path/subdirectory` and any of its
+		 * descendents, capturing the value of the subdirectory in
+		 * {@link MessageAuthorizationContext#getVariables()}</li>
+		 * </ul>
+		 *
+		 * <p>
+		 * A more comprehensive list can be found at {@link PathPattern}.
+		 *
+		 * <p>
+		 * A dot-based message pattern is also supported when configuring a
+		 * {@link PathPatternParser} using
+		 * {@link PathPatternMessageMatcher#withPathPatternParser}
+		 * @param type the message type to match
+		 * @param pattern the destination pattern to match
+		 * @return the {@link PathPatternMessageMatcher.Builder} for more configuration
+		 */
+		public PathPatternMessageMatcher matcher(@Nullable SimpMessageType type, String pattern) {
+			Assert.notNull(pattern, "pattern must not be null");
 			PathPattern pathPattern = this.parser.parse(pattern);
-			PathPatternMessageMatcher matcher = new PathPatternMessageMatcher(pathPattern, this.parser);
-			if (this.messageTypeMatcher != ANY_MESSAGE) {
-				matcher.setMessageTypeMatcher(this.messageTypeMatcher);
+			PathPatternMessageMatcher matcher = new PathPatternMessageMatcher(pathPattern,
+					this.parser.getPathOptions());
+			if (type != null) {
+				matcher.setMessageTypeMatcher(new SimpMessageTypeMatcher(type));
 			}
 			return matcher;
 		}
 
-		public PathPatternMessageMatcher matcher(String pattern, SimpMessageType type) {
-			Assert.notNull(type, "Type must not be null");
-			this.messageTypeMatcher = new SimpMessageTypeMatcher(type);
-
-			return matcher(pattern);
-		}
-
 	}
 
 }

messaging/src/main/java/org/springframework/security/messaging/util/matcher/SimpDestinationMessageMatcher.java

@@ -125,6 +125,12 @@ public boolean matches(Message<?> message) {
 		return destination != null && this.matcher.match(this.pattern, destination);
 	}
 
+	@Override
+	public MatchResult matcher(Message<?> message) {
+		boolean match = matches(message);
+		return (!match) ? MatchResult.notMatch() : MatchResult.match(extractPathVariables(message));
+	}
+
 	public Map<String, String> extractPathVariables(Message<?> message) {
 		final String destination = SimpMessageHeaderAccessor.getDestination(message.getHeaders());
 		return (destination != null) ? this.matcher.extractUriTemplateVariables(this.pattern, destination)

messaging/src/test/java/org/springframework/security/messaging/access/intercept/MessageMatcherDelegatingAuthorizationManagerTests.java

@@ -80,13 +80,13 @@ void checkWhenAnyMessageHasRoleThenRequires() {
 
 	@Test
 	void checkWhenSimpDestinationMatchesThenUses() {
-		AuthorizationManager<Message<?>> authorizationManager = builder().simpDestMatchers("/destination")
+		AuthorizationManager<Message<?>> authorizationManager = builder().simpDestMatchers("destination")
 			.permitAll()
 			.anyMessage()
 			.denyAll()
 			.build();
 		MessageHeaders headers = new MessageHeaders(
-				Map.of(SimpMessageHeaderAccessor.DESTINATION_HEADER, "/destination"));
+				Map.of(SimpMessageHeaderAccessor.DESTINATION_HEADER, "destination"));
 		Message<?> message = new GenericMessage<>(new Object(), headers);
 		assertThat(authorizationManager.check(mock(Supplier.class), message).isGranted()).isTrue();
 	}
@@ -101,7 +101,7 @@ void checkWhenNullDestinationHeaderMatchesThenUses() {
 		Message<?> message = new GenericMessage<>(new Object());
 		assertThat(authorizationManager.check(mock(Supplier.class), message).isGranted()).isTrue();
 		MessageHeaders headers = new MessageHeaders(
-				Map.of(SimpMessageHeaderAccessor.DESTINATION_HEADER, "/destination"));
+				Map.of(SimpMessageHeaderAccessor.DESTINATION_HEADER, "destination"));
 		message = new GenericMessage<>(new Object(), headers);
 		assertThat(authorizationManager.check(mock(Supplier.class), message).isGranted()).isFalse();
 	}
@@ -122,13 +122,13 @@ void checkWhenSimpTypeMatchesThenUses() {
 	// gh-12540
 	@Test
 	void checkWhenSimpDestinationMatchesThenVariablesExtracted() {
-		AuthorizationManager<Message<?>> authorizationManager = builder().simpDestMatchers("/destination/*/{id}")
+		AuthorizationManager<Message<?>> authorizationManager = builder().simpDestMatchers("destination/{id}")
 			.access(variable("id").isEqualTo("3"))
 			.anyMessage()
 			.denyAll()
 			.build();
 		MessageHeaders headers = new MessageHeaders(
-				Map.of(SimpMessageHeaderAccessor.DESTINATION_HEADER, "/destination/sub/3"));
+				Map.of(SimpMessageHeaderAccessor.DESTINATION_HEADER, "destination/3"));
 		Message<?> message = new GenericMessage<>(new Object(), headers);
 		assertThat(authorizationManager.check(mock(Supplier.class), message).isGranted()).isTrue();
 	}
@@ -178,7 +178,13 @@ private Builder variable(String name) {
 
 	}
 
-	private record Builder(String name) {
+	private static final class Builder {
+
+		private final String name;
+
+		private Builder(String name) {
+			this.name = name;
+		}
 
 		AuthorizationManager<MessageAuthorizationContext<?>> isEqualTo(String value) {
 			return (authentication, object) -> {