Skip to content

Commit 6d56f36

Browse files
jzheauxjzheaux
committed
PathPatternMessageMatcher Polish
Issue spring-projectsgh-16500 Signed-off-by: Josh Cummings <[email protected]>
1 parent 8e391f5 commit 6d56f36

9 files changed

+49
-126
lines changed

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

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,7 @@
1717
package org.springframework.security.messaging.access.intercept;
1818

1919
import java.util.ArrayList;
20-
import java.util.Arrays;
2120
import java.util.List;
22-
import java.util.Map;
2321
import java.util.function.Supplier;
2422

2523
import org.apache.commons.logging.Log;
@@ -98,9 +96,6 @@ private MessageAuthorizationContext<?> authorizationContext(MessageMatcher<?> ma
9896
return new MessageAuthorizationContext<>(message, matchResult.getVariables());
9997
}
10098

101-
if (matcher instanceof Builder.LazySimpDestinationMessageMatcher pathMatcher) {
102-
return new MessageAuthorizationContext<>(message, pathMatcher.extractPathVariables(message));
103-
}
10499
return new MessageAuthorizationContext<>(message);
105100
}
106101

@@ -208,7 +203,7 @@ private Builder.Constraint simpDestMatchers(SimpMessageType type, String... patt
208203
List<MessageMatcher<?>> matchers = new ArrayList<>(patterns.length);
209204
for (String pattern : patterns) {
210205
MessageMatcher<Object> matcher = MessageMatcherFactory.usesPathPatterns()
211-
? MessageMatcherFactory.matcher(pattern, type)
206+
? MessageMatcherFactory.matcher(type, pattern)
212207
: new LazySimpDestinationMessageMatcher(pattern, type);
213208
matchers.add(matcher);
214209
}
@@ -254,7 +249,9 @@ public Builder simpDestPathMatcher(Supplier<PathMatcher> pathMatcher) {
254249
*/
255250
public Builder.Constraint matchers(MessageMatcher<?>... matchers) {
256251
List<MessageMatcher<?>> builders = new ArrayList<>(matchers.length);
257-
builders.addAll(Arrays.asList(matchers));
252+
for (MessageMatcher<?> matcher : matchers) {
253+
builders.add(matcher);
254+
}
258255
return new Builder.Constraint(builders);
259256
}
260257

@@ -419,8 +416,9 @@ public boolean matches(Message<?> message) {
419416
return this.delegate.get().matches(message);
420417
}
421418

422-
Map<String, String> extractPathVariables(Message<?> message) {
423-
return this.delegate.get().extractPathVariables(message);
419+
@Override
420+
public MatchResult matcher(Message<?> message) {
421+
return this.delegate.get().matcher(message);
424422
}
425423

426424
}
@@ -433,7 +431,7 @@ private static final class Entry<T> {
433431

434432
private final T entry;
435433

436-
Entry(MessageMatcher<?> requestMatcher, T entry) {
434+
Entry(MessageMatcher requestMatcher, T entry) {
437435
this.messageMatcher = requestMatcher;
438436
this.entry = entry;
439437
}

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,11 +33,11 @@ public static boolean usesPathPatterns() {
3333
}
3434

3535
public static MessageMatcher<?> matcher(String destination) {
36-
return builder.matcher(destination);
36+
return matcher(null, destination);
3737
}
3838

39-
public static MessageMatcher<Object> matcher(String destination, SimpMessageType type) {
40-
return (type != null) ? builder.matcher(destination, type) : builder.matcher(destination);
39+
public static MessageMatcher<Object> matcher(SimpMessageType type, String destination) {
40+
return builder.matcher(type, destination);
4141
}
4242

4343
private MessageMatcherFactory() {

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

Lines changed: 15 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import java.util.Collections;
2020

2121
import org.springframework.http.server.PathContainer;
22+
import org.springframework.lang.Nullable;
2223
import org.springframework.messaging.Message;
2324
import org.springframework.messaging.simp.SimpMessageHeaderAccessor;
2425
import org.springframework.messaging.simp.SimpMessageType;
@@ -40,16 +41,16 @@ public final class PathPatternMessageMatcher implements MessageMatcher<Object> {
4041

4142
private final PathPattern pattern;
4243

43-
private final PathPatternParser parser;
44+
private final PathContainer.Options options;
4445

4546
/**
4647
* The {@link MessageMatcher} that determines if the type matches. If the type was
4748
* null, this matcher will match every Message.
4849
*/
4950
private MessageMatcher<Object> messageTypeMatcher = ANY_MESSAGE;
5051

51-
private PathPatternMessageMatcher(PathPattern pattern, PathPatternParser parser) {
52-
this.parser = parser;
52+
private PathPatternMessageMatcher(PathPattern pattern, PathContainer.Options options) {
53+
this.options = options;
5354
this.pattern = pattern;
5455
}
5556

@@ -78,17 +79,7 @@ void setMessageTypeMatcher(MessageMatcher<Object> messageTypeMatcher) {
7879
*/
7980
@Override
8081
public boolean matches(Message<?> message) {
81-
if (!this.messageTypeMatcher.matches(message)) {
82-
return false;
83-
}
84-
85-
String destination = getDestination(message);
86-
if (destination == null) {
87-
return false;
88-
}
89-
90-
PathContainer destinationPathContainer = PathContainer.parsePath(destination, this.parser.getPathOptions());
91-
return this.pattern.matches(destinationPathContainer);
82+
return matcher(message).isMatch();
9283
}
9384

9485
/**
@@ -109,7 +100,7 @@ public MatchResult matcher(Message<?> message) {
109100
return MatchResult.notMatch();
110101
}
111102

112-
PathContainer destinationPathContainer = PathContainer.parsePath(destination, this.parser.getPathOptions());
103+
PathContainer destinationPathContainer = PathContainer.parsePath(destination, this.options);
113104
PathPattern.PathMatchInfo pathMatchInfo = this.pattern.matchAndExtract(destinationPathContainer);
114105

115106
return (pathMatchInfo != null) ? MatchResult.match(pathMatchInfo.getUriVariables()) : MatchResult.notMatch();
@@ -123,29 +114,25 @@ public static class Builder {
123114

124115
private final PathPatternParser parser;
125116

126-
private MessageMatcher<Object> messageTypeMatcher = ANY_MESSAGE;
127-
128117
Builder(PathPatternParser parser) {
129118
this.parser = parser;
130119
}
131120

132121
public PathPatternMessageMatcher matcher(String pattern) {
133-
Assert.notNull(pattern, "Pattern must not be null");
122+
return matcher(null, pattern);
123+
}
124+
125+
public PathPatternMessageMatcher matcher(@Nullable SimpMessageType type, String pattern) {
126+
Assert.notNull(pattern, "pattern must not be null");
134127
PathPattern pathPattern = this.parser.parse(pattern);
135-
PathPatternMessageMatcher matcher = new PathPatternMessageMatcher(pathPattern, this.parser);
136-
if (this.messageTypeMatcher != ANY_MESSAGE) {
137-
matcher.setMessageTypeMatcher(this.messageTypeMatcher);
128+
PathPatternMessageMatcher matcher = new PathPatternMessageMatcher(pathPattern,
129+
this.parser.getPathOptions());
130+
if (type != null) {
131+
matcher.setMessageTypeMatcher(new SimpMessageTypeMatcher(type));
138132
}
139133
return matcher;
140134
}
141135

142-
public PathPatternMessageMatcher matcher(String pattern, SimpMessageType type) {
143-
Assert.notNull(type, "Type must not be null");
144-
this.messageTypeMatcher = new SimpMessageTypeMatcher(type);
145-
146-
return matcher(pattern);
147-
}
148-
149136
}
150137

151138
}

‎messaging/src/main/java/org/springframework/security/messaging/util/matcher/PathPatternMessageMatcherBuilderFactoryBean.java

Lines changed: 1 addition & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
package org.springframework.security.messaging.util.matcher;
1818

1919
import org.springframework.beans.factory.FactoryBean;
20-
import org.springframework.web.util.pattern.PathPatternParser;
2120

2221
/**
2322
* Use this factory bean to configure the {@link PathPatternMessageMatcher.Builder} bean
@@ -30,20 +29,9 @@
3029
*/
3130
public class PathPatternMessageMatcherBuilderFactoryBean implements FactoryBean<PathPatternMessageMatcher.Builder> {
3231

33-
private final PathPatternParser parser;
34-
35-
public PathPatternMessageMatcherBuilderFactoryBean() {
36-
this(null);
37-
}
38-
39-
public PathPatternMessageMatcherBuilderFactoryBean(PathPatternParser parser) {
40-
this.parser = parser;
41-
}
42-
4332
@Override
4433
public PathPatternMessageMatcher.Builder getObject() throws Exception {
45-
return (this.parser != null) ? PathPatternMessageMatcher.withPathPatternParser(this.parser)
46-
: PathPatternMessageMatcher.withDefaults();
34+
return PathPatternMessageMatcher.withDefaults();
4735
}
4836

4937
@Override

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,12 @@ public boolean matches(Message<?> message) {
125125
return destination != null && this.matcher.match(this.pattern, destination);
126126
}
127127

128+
@Override
129+
public MatchResult matcher(Message<?> message) {
130+
boolean match = matches(message);
131+
return (!match) ? MatchResult.notMatch() : MatchResult.match(extractPathVariables(message));
132+
}
133+
128134
public Map<String, String> extractPathVariables(Message<?> message) {
129135
final String destination = SimpMessageHeaderAccessor.getDestination(message.getHeaders());
130136
return (destination != null) ? this.matcher.extractUriTemplateVariables(this.pattern, destination)

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

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -80,13 +80,13 @@ void checkWhenAnyMessageHasRoleThenRequires() {
8080

8181
@Test
8282
void checkWhenSimpDestinationMatchesThenUses() {
83-
AuthorizationManager<Message<?>> authorizationManager = builder().simpDestMatchers("/destination")
83+
AuthorizationManager<Message<?>> authorizationManager = builder().simpDestMatchers("destination")
8484
.permitAll()
8585
.anyMessage()
8686
.denyAll()
8787
.build();
8888
MessageHeaders headers = new MessageHeaders(
89-
Map.of(SimpMessageHeaderAccessor.DESTINATION_HEADER, "/destination"));
89+
Map.of(SimpMessageHeaderAccessor.DESTINATION_HEADER, "destination"));
9090
Message<?> message = new GenericMessage<>(new Object(), headers);
9191
assertThat(authorizationManager.check(mock(Supplier.class), message).isGranted()).isTrue();
9292
}
@@ -101,7 +101,7 @@ void checkWhenNullDestinationHeaderMatchesThenUses() {
101101
Message<?> message = new GenericMessage<>(new Object());
102102
assertThat(authorizationManager.check(mock(Supplier.class), message).isGranted()).isTrue();
103103
MessageHeaders headers = new MessageHeaders(
104-
Map.of(SimpMessageHeaderAccessor.DESTINATION_HEADER, "/destination"));
104+
Map.of(SimpMessageHeaderAccessor.DESTINATION_HEADER, "destination"));
105105
message = new GenericMessage<>(new Object(), headers);
106106
assertThat(authorizationManager.check(mock(Supplier.class), message).isGranted()).isFalse();
107107
}
@@ -122,13 +122,13 @@ void checkWhenSimpTypeMatchesThenUses() {
122122
// gh-12540
123123
@Test
124124
void checkWhenSimpDestinationMatchesThenVariablesExtracted() {
125-
AuthorizationManager<Message<?>> authorizationManager = builder().simpDestMatchers("/destination/*/{id}")
125+
AuthorizationManager<Message<?>> authorizationManager = builder().simpDestMatchers("destination/{id}")
126126
.access(variable("id").isEqualTo("3"))
127127
.anyMessage()
128128
.denyAll()
129129
.build();
130130
MessageHeaders headers = new MessageHeaders(
131-
Map.of(SimpMessageHeaderAccessor.DESTINATION_HEADER, "/destination/sub/3"));
131+
Map.of(SimpMessageHeaderAccessor.DESTINATION_HEADER, "destination/3"));
132132
Message<?> message = new GenericMessage<>(new Object(), headers);
133133
assertThat(authorizationManager.check(mock(Supplier.class), message).isGranted()).isTrue();
134134
}
@@ -178,7 +178,13 @@ private Builder variable(String name) {
178178

179179
}
180180

181-
private record Builder(String name) {
181+
private static final class Builder {
182+
183+
private final String name;
184+
185+
private Builder(String name) {
186+
this.name = name;
187+
}
182188

183189
AuthorizationManager<MessageAuthorizationContext<?>> isEqualTo(String value) {
184190
return (authentication, object) -> {

‎messaging/src/test/java/org/springframework/security/messaging/util/matcher/PathPatternMessageMatcherBuilderFactoryBeanTests.java

Lines changed: 0 additions & 62 deletions
This file was deleted.

‎messaging/src/test/java/org/springframework/security/messaging/util/matcher/PathPatternMessageMatcherTests.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ void matchesFalseWithDotSeparatorAndAdditionalWildcardPathSegment() {
8484

8585
@Test
8686
void matchesFalseWithDifferentMessageType() {
87-
this.matcher = PathPatternMessageMatcher.withDefaults().matcher("/match", SimpMessageType.MESSAGE);
87+
this.matcher = PathPatternMessageMatcher.withDefaults().matcher(SimpMessageType.MESSAGE, "/match");
8888
this.messageBuilder.setHeader(SimpMessageHeaderAccessor.MESSAGE_TYPE_HEADER, SimpMessageType.DISCONNECT);
8989
this.messageBuilder.setHeader(SimpMessageHeaderAccessor.DESTINATION_HEADER, "/match");
9090

@@ -93,15 +93,15 @@ void matchesFalseWithDifferentMessageType() {
9393

9494
@Test
9595
public void matchesTrueMessageType() {
96-
this.matcher = PathPatternMessageMatcher.withDefaults().matcher("/match", SimpMessageType.MESSAGE);
96+
this.matcher = PathPatternMessageMatcher.withDefaults().matcher(SimpMessageType.MESSAGE, "/match");
9797
this.messageBuilder.setHeader(SimpMessageHeaderAccessor.DESTINATION_HEADER, "/match");
9898
this.messageBuilder.setHeader(SimpMessageHeaderAccessor.MESSAGE_TYPE_HEADER, SimpMessageType.MESSAGE);
9999
assertThat(this.matcher.matches(this.messageBuilder.build())).isTrue();
100100
}
101101

102102
@Test
103103
public void matchesTrueSubscribeType() {
104-
this.matcher = PathPatternMessageMatcher.withDefaults().matcher("/match", SimpMessageType.SUBSCRIBE);
104+
this.matcher = PathPatternMessageMatcher.withDefaults().matcher(SimpMessageType.SUBSCRIBE, "/match");
105105
this.messageBuilder.setHeader(SimpMessageHeaderAccessor.DESTINATION_HEADER, "/match");
106106
this.messageBuilder.setHeader(SimpMessageHeaderAccessor.MESSAGE_TYPE_HEADER, SimpMessageType.SUBSCRIBE);
107107
assertThat(this.matcher.matches(this.messageBuilder.build())).isTrue();

‎messaging/src/test/java/org/springframework/security/messaging/util/matcher/SimpDestinationMessageMatcherTests.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,7 @@ public void typeConstructorParameterIsTransmitted() {
131131
}
132132

133133
@Test
134-
void illegalStateExceptionThrown_onExtractPathVariables_whenNoMatch() {
134+
public void extractPathVariablesWhenNoMatchThenIllegalState() {
135135
this.matcher = new SimpDestinationMessageMatcher("/nomatch");
136136
this.messageBuilder.setHeader(SimpMessageHeaderAccessor.DESTINATION_HEADER, "/destination/1");
137137
assertThatIllegalStateException()

0 commit comments

Comments
 (0)