Skip to content

Commit f541bce

Browse files
jzheauxjzheaux
committed
Polish AuthorizationAdvisorProxyFactory
- Ensure Reasonable Defaults - Simplify Construction Issue gh-14596
1 parent 6073cd9 commit f541bce

File tree

3 files changed

+61
-90
lines changed

3 files changed

+61
-90
lines changed

‎config/src/main/java/org/springframework/security/config/annotation/method/configuration/AuthorizationProxyConfiguration.java

+3-3
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@
2525
import org.springframework.context.annotation.Bean;
2626
import org.springframework.context.annotation.Configuration;
2727
import org.springframework.context.annotation.Role;
28-
import org.springframework.core.annotation.AnnotationAwareOrderComparator;
2928
import org.springframework.security.authorization.AuthorizationAdvisorProxyFactory;
3029
import org.springframework.security.authorization.method.AuthorizationAdvisor;
3130

@@ -37,8 +36,9 @@ final class AuthorizationProxyConfiguration implements AopInfrastructureBean {
3736
static AuthorizationAdvisorProxyFactory authorizationProxyFactory(ObjectProvider<AuthorizationAdvisor> provider) {
3837
List<AuthorizationAdvisor> advisors = new ArrayList<>();
3938
provider.forEach(advisors::add);
40-
AnnotationAwareOrderComparator.sort(advisors);
41-
return new AuthorizationAdvisorProxyFactory(advisors);
39+
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
40+
factory.setAdvisors(advisors);
41+
return factory;
4242
}
4343

4444
}

‎core/src/main/java/org/springframework/security/authorization/AuthorizationAdvisorProxyFactory.java

+36-24
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,10 @@
4040
import org.springframework.aop.framework.ProxyFactory;
4141
import org.springframework.core.annotation.AnnotationAwareOrderComparator;
4242
import org.springframework.security.authorization.method.AuthorizationAdvisor;
43+
import org.springframework.security.authorization.method.AuthorizationManagerAfterMethodInterceptor;
44+
import org.springframework.security.authorization.method.AuthorizationManagerBeforeMethodInterceptor;
45+
import org.springframework.security.authorization.method.PostFilterAuthorizationMethodInterceptor;
46+
import org.springframework.security.authorization.method.PreFilterAuthorizationMethodInterceptor;
4347
import org.springframework.util.ClassUtils;
4448

4549
/**
@@ -71,31 +75,15 @@
7175
*/
7276
public final class AuthorizationAdvisorProxyFactory implements AuthorizationProxyFactory {
7377

74-
private final Collection<AuthorizationAdvisor> advisors;
78+
private List<AuthorizationAdvisor> advisors = new ArrayList<>();
7579

76-
public AuthorizationAdvisorProxyFactory(AuthorizationAdvisor... advisors) {
77-
this.advisors = List.of(advisors);
78-
}
79-
80-
public AuthorizationAdvisorProxyFactory(Collection<AuthorizationAdvisor> advisors) {
81-
this.advisors = List.copyOf(advisors);
82-
}
83-
84-
/**
85-
* Create a new {@link AuthorizationAdvisorProxyFactory} that includes the given
86-
* advisors in addition to any advisors {@code this} instance already has.
87-
*
88-
* <p>
89-
* All advisors are re-sorted by their advisor order.
90-
* @param advisors the advisors to add
91-
* @return a new {@link AuthorizationAdvisorProxyFactory} instance
92-
*/
93-
public AuthorizationAdvisorProxyFactory withAdvisors(AuthorizationAdvisor... advisors) {
94-
List<AuthorizationAdvisor> merged = new ArrayList<>(this.advisors.size() + advisors.length);
95-
merged.addAll(this.advisors);
96-
merged.addAll(List.of(advisors));
97-
AnnotationAwareOrderComparator.sort(merged);
98-
return new AuthorizationAdvisorProxyFactory(merged);
80+
public AuthorizationAdvisorProxyFactory() {
81+
List<AuthorizationAdvisor> advisors = new ArrayList<>();
82+
advisors.add(AuthorizationManagerBeforeMethodInterceptor.preAuthorize());
83+
advisors.add(AuthorizationManagerAfterMethodInterceptor.postAuthorize());
84+
advisors.add(new PreFilterAuthorizationMethodInterceptor());
85+
advisors.add(new PostFilterAuthorizationMethodInterceptor());
86+
setAdvisors(advisors);
9987
}
10088

10189
/**
@@ -165,6 +153,30 @@ public Object proxy(Object target) {
165153
return factory.getProxy();
166154
}
167155

156+
/**
157+
* Add advisors that should be included to each proxy created.
158+
*
159+
* <p>
160+
* All advisors are re-sorted by their advisor order.
161+
* @param advisors the advisors to add
162+
*/
163+
public void setAdvisors(AuthorizationAdvisor... advisors) {
164+
this.advisors = new ArrayList<>(List.of(advisors));
165+
AnnotationAwareOrderComparator.sort(this.advisors);
166+
}
167+
168+
/**
169+
* Add advisors that should be included to each proxy created.
170+
*
171+
* <p>
172+
* All advisors are re-sorted by their advisor order.
173+
* @param advisors the advisors to add
174+
*/
175+
public void setAdvisors(Collection<AuthorizationAdvisor> advisors) {
176+
this.advisors = new ArrayList<>(advisors);
177+
AnnotationAwareOrderComparator.sort(this.advisors);
178+
}
179+
168180
@SuppressWarnings("unchecked")
169181
private <T> T proxyCast(T target) {
170182
return (T) proxy(target);

‎core/src/test/java/org/springframework/security/authorization/AuthorizationAdvisorProxyFactoryTests.java

+22-63
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,6 @@
4141
import org.springframework.security.access.prepost.PreAuthorize;
4242
import org.springframework.security.authentication.TestAuthentication;
4343
import org.springframework.security.authorization.method.AuthorizationAdvisor;
44-
import org.springframework.security.authorization.method.AuthorizationManagerBeforeMethodInterceptor;
4544
import org.springframework.security.core.Authentication;
4645
import org.springframework.security.core.context.SecurityContextHolder;
4746

@@ -65,9 +64,7 @@ public class AuthorizationAdvisorProxyFactoryTests {
6564
@Test
6665
public void proxyWhenPreAuthorizeThenHonors() {
6766
SecurityContextHolder.getContext().setAuthentication(this.user);
68-
AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor
69-
.preAuthorize();
70-
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
67+
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
7168
Flight flight = new Flight();
7269
assertThat(flight.getAltitude()).isEqualTo(35000d);
7370
Flight secured = proxy(factory, flight);
@@ -78,9 +75,7 @@ public void proxyWhenPreAuthorizeThenHonors() {
7875
@Test
7976
public void proxyWhenPreAuthorizeOnInterfaceThenHonors() {
8077
SecurityContextHolder.getContext().setAuthentication(this.user);
81-
AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor
82-
.preAuthorize();
83-
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
78+
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
8479
assertThat(this.alan.getFirstName()).isEqualTo("alan");
8580
User secured = proxy(factory, this.alan);
8681
assertThatExceptionOfType(AccessDeniedException.class).isThrownBy(secured::getFirstName);
@@ -94,9 +89,7 @@ public void proxyWhenPreAuthorizeOnInterfaceThenHonors() {
9489
@Test
9590
public void proxyWhenPreAuthorizeOnRecordThenHonors() {
9691
SecurityContextHolder.getContext().setAuthentication(this.user);
97-
AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor
98-
.preAuthorize();
99-
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
92+
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
10093
HasSecret repo = new Repository("secret");
10194
assertThat(repo.secret()).isEqualTo("secret");
10295
HasSecret secured = proxy(factory, repo);
@@ -109,9 +102,7 @@ public void proxyWhenPreAuthorizeOnRecordThenHonors() {
109102
@Test
110103
public void proxyWhenImmutableListThenReturnsSecuredImmutableList() {
111104
SecurityContextHolder.getContext().setAuthentication(this.user);
112-
AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor
113-
.preAuthorize();
114-
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
105+
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
115106
List<Flight> flights = List.of(this.flight);
116107
List<Flight> secured = proxy(factory, flights);
117108
secured.forEach(
@@ -123,9 +114,7 @@ public void proxyWhenImmutableListThenReturnsSecuredImmutableList() {
123114
@Test
124115
public void proxyWhenImmutableSetThenReturnsSecuredImmutableSet() {
125116
SecurityContextHolder.getContext().setAuthentication(this.user);
126-
AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor
127-
.preAuthorize();
128-
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
117+
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
129118
Set<Flight> flights = Set.of(this.flight);
130119
Set<Flight> secured = proxy(factory, flights);
131120
secured.forEach(
@@ -137,9 +126,7 @@ public void proxyWhenImmutableSetThenReturnsSecuredImmutableSet() {
137126
@Test
138127
public void proxyWhenQueueThenReturnsSecuredQueue() {
139128
SecurityContextHolder.getContext().setAuthentication(this.user);
140-
AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor
141-
.preAuthorize();
142-
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
129+
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
143130
Queue<Flight> flights = new LinkedList<>(List.of(this.flight));
144131
Queue<Flight> secured = proxy(factory, flights);
145132
assertThat(flights.size()).isEqualTo(secured.size());
@@ -151,9 +138,7 @@ public void proxyWhenQueueThenReturnsSecuredQueue() {
151138
@Test
152139
public void proxyWhenImmutableSortedSetThenReturnsSecuredImmutableSortedSet() {
153140
SecurityContextHolder.getContext().setAuthentication(this.user);
154-
AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor
155-
.preAuthorize();
156-
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
141+
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
157142
SortedSet<User> users = Collections.unmodifiableSortedSet(new TreeSet<>(Set.of(this.alan)));
158143
SortedSet<User> secured = proxy(factory, users);
159144
secured
@@ -165,9 +150,7 @@ public void proxyWhenImmutableSortedSetThenReturnsSecuredImmutableSortedSet() {
165150
@Test
166151
public void proxyWhenImmutableSortedMapThenReturnsSecuredImmutableSortedMap() {
167152
SecurityContextHolder.getContext().setAuthentication(this.user);
168-
AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor
169-
.preAuthorize();
170-
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
153+
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
171154
SortedMap<String, User> users = Collections
172155
.unmodifiableSortedMap(new TreeMap<>(Map.of(this.alan.getId(), this.alan)));
173156
SortedMap<String, User> secured = proxy(factory, users);
@@ -180,9 +163,7 @@ public void proxyWhenImmutableSortedMapThenReturnsSecuredImmutableSortedMap() {
180163
@Test
181164
public void proxyWhenImmutableMapThenReturnsSecuredImmutableMap() {
182165
SecurityContextHolder.getContext().setAuthentication(this.user);
183-
AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor
184-
.preAuthorize();
185-
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
166+
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
186167
Map<String, User> users = Map.of(this.alan.getId(), this.alan);
187168
Map<String, User> secured = proxy(factory, users);
188169
secured.forEach(
@@ -194,9 +175,7 @@ public void proxyWhenImmutableMapThenReturnsSecuredImmutableMap() {
194175
@Test
195176
public void proxyWhenMutableListThenReturnsSecuredMutableList() {
196177
SecurityContextHolder.getContext().setAuthentication(this.user);
197-
AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor
198-
.preAuthorize();
199-
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
178+
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
200179
List<Flight> flights = new ArrayList<>(List.of(this.flight));
201180
List<Flight> secured = proxy(factory, flights);
202181
secured.forEach(
@@ -208,9 +187,7 @@ public void proxyWhenMutableListThenReturnsSecuredMutableList() {
208187
@Test
209188
public void proxyWhenMutableSetThenReturnsSecuredMutableSet() {
210189
SecurityContextHolder.getContext().setAuthentication(this.user);
211-
AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor
212-
.preAuthorize();
213-
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
190+
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
214191
Set<Flight> flights = new HashSet<>(Set.of(this.flight));
215192
Set<Flight> secured = proxy(factory, flights);
216193
secured.forEach(
@@ -222,9 +199,7 @@ public void proxyWhenMutableSetThenReturnsSecuredMutableSet() {
222199
@Test
223200
public void proxyWhenMutableSortedSetThenReturnsSecuredMutableSortedSet() {
224201
SecurityContextHolder.getContext().setAuthentication(this.user);
225-
AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor
226-
.preAuthorize();
227-
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
202+
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
228203
SortedSet<User> users = new TreeSet<>(Set.of(this.alan));
229204
SortedSet<User> secured = proxy(factory, users);
230205
secured.forEach((u) -> assertThatExceptionOfType(AccessDeniedException.class).isThrownBy(u::getFirstName));
@@ -235,9 +210,7 @@ public void proxyWhenMutableSortedSetThenReturnsSecuredMutableSortedSet() {
235210
@Test
236211
public void proxyWhenMutableSortedMapThenReturnsSecuredMutableSortedMap() {
237212
SecurityContextHolder.getContext().setAuthentication(this.user);
238-
AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor
239-
.preAuthorize();
240-
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
213+
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
241214
SortedMap<String, User> users = new TreeMap<>(Map.of(this.alan.getId(), this.alan));
242215
SortedMap<String, User> secured = proxy(factory, users);
243216
secured.forEach((id, u) -> assertThatExceptionOfType(AccessDeniedException.class).isThrownBy(u::getFirstName));
@@ -248,9 +221,7 @@ public void proxyWhenMutableSortedMapThenReturnsSecuredMutableSortedMap() {
248221
@Test
249222
public void proxyWhenMutableMapThenReturnsSecuredMutableMap() {
250223
SecurityContextHolder.getContext().setAuthentication(this.user);
251-
AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor
252-
.preAuthorize();
253-
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
224+
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
254225
Map<String, User> users = new HashMap<>(Map.of(this.alan.getId(), this.alan));
255226
Map<String, User> secured = proxy(factory, users);
256227
secured.forEach((id, u) -> assertThatExceptionOfType(AccessDeniedException.class).isThrownBy(u::getFirstName));
@@ -261,9 +232,7 @@ public void proxyWhenMutableMapThenReturnsSecuredMutableMap() {
261232
@Test
262233
public void proxyWhenPreAuthorizeForOptionalThenHonors() {
263234
SecurityContextHolder.getContext().setAuthentication(this.user);
264-
AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor
265-
.preAuthorize();
266-
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
235+
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
267236
Optional<Flight> flights = Optional.of(this.flight);
268237
assertThat(flights.get().getAltitude()).isEqualTo(35000d);
269238
Optional<Flight> secured = proxy(factory, flights);
@@ -274,9 +243,7 @@ public void proxyWhenPreAuthorizeForOptionalThenHonors() {
274243
@Test
275244
public void proxyWhenPreAuthorizeForStreamThenHonors() {
276245
SecurityContextHolder.getContext().setAuthentication(this.user);
277-
AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor
278-
.preAuthorize();
279-
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
246+
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
280247
Stream<Flight> flights = Stream.of(this.flight);
281248
Stream<Flight> secured = proxy(factory, flights);
282249
assertThatExceptionOfType(AccessDeniedException.class).isThrownBy(() -> secured.forEach(Flight::getAltitude));
@@ -286,9 +253,7 @@ public void proxyWhenPreAuthorizeForStreamThenHonors() {
286253
@Test
287254
public void proxyWhenPreAuthorizeForArrayThenHonors() {
288255
SecurityContextHolder.getContext().setAuthentication(this.user);
289-
AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor
290-
.preAuthorize();
291-
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
256+
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
292257
Flight[] flights = { this.flight };
293258
Flight[] secured = proxy(factory, flights);
294259
assertThatExceptionOfType(AccessDeniedException.class).isThrownBy(secured[0]::getAltitude);
@@ -298,9 +263,7 @@ public void proxyWhenPreAuthorizeForArrayThenHonors() {
298263
@Test
299264
public void proxyWhenPreAuthorizeForIteratorThenHonors() {
300265
SecurityContextHolder.getContext().setAuthentication(this.user);
301-
AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor
302-
.preAuthorize();
303-
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
266+
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
304267
Iterator<Flight> flights = List.of(this.flight).iterator();
305268
Iterator<Flight> secured = proxy(factory, flights);
306269
assertThatExceptionOfType(AccessDeniedException.class).isThrownBy(() -> secured.next().getAltitude());
@@ -310,9 +273,7 @@ public void proxyWhenPreAuthorizeForIteratorThenHonors() {
310273
@Test
311274
public void proxyWhenPreAuthorizeForIterableThenHonors() {
312275
SecurityContextHolder.getContext().setAuthentication(this.user);
313-
AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor
314-
.preAuthorize();
315-
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
276+
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
316277
Iterable<User> users = new UserRepository();
317278
Iterable<User> secured = proxy(factory, users);
318279
assertThatExceptionOfType(AccessDeniedException.class).isThrownBy(() -> secured.forEach(User::getFirstName));
@@ -321,9 +282,7 @@ public void proxyWhenPreAuthorizeForIterableThenHonors() {
321282

322283
@Test
323284
public void proxyWhenPreAuthorizeForClassThenHonors() {
324-
AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor
325-
.preAuthorize();
326-
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
285+
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
327286
Class<Flight> clazz = proxy(factory, Flight.class);
328287
assertThat(clazz.getSimpleName()).contains("SpringCGLIB$$0");
329288
Flight secured = proxy(factory, this.flight);
@@ -334,12 +293,12 @@ public void proxyWhenPreAuthorizeForClassThenHonors() {
334293
}
335294

336295
@Test
337-
public void withAdvisorsWhenProxyThenVisits() {
296+
public void setAdvisorsWhenProxyThenVisits() {
338297
AuthorizationAdvisor advisor = mock(AuthorizationAdvisor.class);
339298
given(advisor.getAdvice()).willReturn(advisor);
340299
given(advisor.getPointcut()).willReturn(Pointcut.TRUE);
341300
AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
342-
factory = factory.withAdvisors(advisor);
301+
factory.setAdvisors(advisor);
343302
Flight flight = proxy(factory, this.flight);
344303
flight.getAltitude();
345304
verify(advisor, atLeastOnce()).getPointcut();

0 commit comments

Comments
 (0)