Skip to content

Commit 286e958

Browse files
evgeniychebanrwinch
authored andcommitted
@EnableMethodSecurity doesn't resolve Method Security annotations on interfaces through a Proxy
Removed proxy unwrapping in case of resolving Method Security annotations, this cause an issue when interfaces which are implemented by the proxy was skipped, resulting in a missing security checks on those methods. Closes gh-11175
1 parent 5ac5edc commit 286e958

File tree

4 files changed

+88
-4
lines changed

4 files changed

+88
-4
lines changed

core/src/main/java/org/springframework/security/authorization/method/AbstractAuthorizationManagerRegistry.java

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@
2222

2323
import org.aopalliance.intercept.MethodInvocation;
2424

25-
import org.springframework.aop.support.AopUtils;
2625
import org.springframework.core.MethodClassKey;
2726
import org.springframework.lang.NonNull;
2827
import org.springframework.security.authorization.AuthorizationManager;
@@ -46,7 +45,7 @@ abstract class AbstractAuthorizationManagerRegistry {
4645
final AuthorizationManager<MethodInvocation> getManager(MethodInvocation methodInvocation) {
4746
Method method = methodInvocation.getMethod();
4847
Object target = methodInvocation.getThis();
49-
Class<?> targetClass = (target != null) ? AopUtils.getTargetClass(target) : null;
48+
Class<?> targetClass = (target != null) ? target.getClass() : null;
5049
MethodClassKey cacheKey = new MethodClassKey(method, targetClass);
5150
return this.cachedManagers.computeIfAbsent(cacheKey, (k) -> resolveManager(method, targetClass));
5251
}

core/src/main/java/org/springframework/security/authorization/method/AbstractExpressionAttributeRegistry.java

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@
2222

2323
import org.aopalliance.intercept.MethodInvocation;
2424

25-
import org.springframework.aop.support.AopUtils;
2625
import org.springframework.core.MethodClassKey;
2726
import org.springframework.lang.NonNull;
2827

@@ -43,7 +42,7 @@ abstract class AbstractExpressionAttributeRegistry<T extends ExpressionAttribute
4342
final T getAttribute(MethodInvocation mi) {
4443
Method method = mi.getMethod();
4544
Object target = mi.getThis();
46-
Class<?> targetClass = (target != null) ? AopUtils.getTargetClass(target) : null;
45+
Class<?> targetClass = (target != null) ? target.getClass() : null;
4746
return getAttribute(method, targetClass);
4847
}
4948

core/src/test/java/org/springframework/security/authorization/method/PreAuthorizeAuthorizationManagerTests.java

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222

2323
import org.junit.jupiter.api.Test;
2424

25+
import org.springframework.aop.TargetClassAware;
2526
import org.springframework.core.annotation.AnnotationConfigurationException;
2627
import org.springframework.security.access.expression.method.DefaultMethodSecurityExpressionHandler;
2728
import org.springframework.security.access.expression.method.MethodSecurityExpressionHandler;
@@ -133,6 +134,19 @@ public void checkInheritedAnnotationsWhenConflictingThenAnnotationConfigurationE
133134
.isThrownBy(() -> manager.check(authentication, methodInvocation));
134135
}
135136

137+
@Test
138+
public void checkTargetClassAwareWhenInterfaceLevelAnnotationsThenApplies() throws Exception {
139+
MockMethodInvocation methodInvocation = new MockMethodInvocation(new TestTargetClassAware(),
140+
TestTargetClassAware.class, "doSomething");
141+
PreAuthorizeAuthorizationManager manager = new PreAuthorizeAuthorizationManager();
142+
AuthorizationDecision decision = manager.check(TestAuthentication::authenticatedUser, methodInvocation);
143+
assertThat(decision).isNotNull();
144+
assertThat(decision.isGranted()).isFalse();
145+
decision = manager.check(TestAuthentication::authenticatedAdmin, methodInvocation);
146+
assertThat(decision).isNotNull();
147+
assertThat(decision.isGranted()).isTrue();
148+
}
149+
136150
public static class TestClass implements InterfaceAnnotationsOne, InterfaceAnnotationsTwo {
137151

138152
public void doSomething() {
@@ -198,4 +212,33 @@ public interface InterfaceAnnotationsThree {
198212

199213
}
200214

215+
@PreAuthorize("hasRole('ADMIN')")
216+
public interface InterfaceLevelAnnotations {
217+
218+
}
219+
220+
public static class TestTargetClassAware extends TestClass implements TargetClassAware, InterfaceLevelAnnotations {
221+
222+
@Override
223+
public Class<?> getTargetClass() {
224+
return TestClass.class;
225+
}
226+
227+
@Override
228+
public void doSomething() {
229+
super.doSomething();
230+
}
231+
232+
@Override
233+
public String doSomethingString(String s) {
234+
return super.doSomethingString(s);
235+
}
236+
237+
@Override
238+
public void inheritedAnnotations() {
239+
super.inheritedAnnotations();
240+
}
241+
242+
}
243+
201244
}

core/src/test/java/org/springframework/security/authorization/method/SecuredAuthorizationManagerTests.java

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222

2323
import org.junit.jupiter.api.Test;
2424

25+
import org.springframework.aop.TargetClassAware;
2526
import org.springframework.core.annotation.AnnotationConfigurationException;
2627
import org.springframework.security.access.annotation.Secured;
2728
import org.springframework.security.access.intercept.method.MockMethodInvocation;
@@ -127,6 +128,19 @@ public void checkInheritedAnnotationsWhenConflictingThenAnnotationConfigurationE
127128
.isThrownBy(() -> manager.check(authentication, methodInvocation));
128129
}
129130

131+
@Test
132+
public void checkTargetClassAwareWhenInterfaceLevelAnnotationsThenApplies() throws Exception {
133+
MockMethodInvocation methodInvocation = new MockMethodInvocation(new TestTargetClassAware(),
134+
TestTargetClassAware.class, "doSomething");
135+
SecuredAuthorizationManager manager = new SecuredAuthorizationManager();
136+
AuthorizationDecision decision = manager.check(TestAuthentication::authenticatedUser, methodInvocation);
137+
assertThat(decision).isNotNull();
138+
assertThat(decision.isGranted()).isFalse();
139+
decision = manager.check(TestAuthentication::authenticatedAdmin, methodInvocation);
140+
assertThat(decision).isNotNull();
141+
assertThat(decision.isGranted()).isTrue();
142+
}
143+
130144
public static class TestClass implements InterfaceAnnotationsOne, InterfaceAnnotationsTwo {
131145

132146
public void doSomething() {
@@ -192,4 +206,33 @@ public interface InterfaceAnnotationsThree {
192206

193207
}
194208

209+
@Secured("ROLE_ADMIN")
210+
public interface InterfaceLevelAnnotations {
211+
212+
}
213+
214+
public static class TestTargetClassAware extends TestClass implements TargetClassAware, InterfaceLevelAnnotations {
215+
216+
@Override
217+
public Class<?> getTargetClass() {
218+
return TestClass.class;
219+
}
220+
221+
@Override
222+
public void doSomething() {
223+
super.doSomething();
224+
}
225+
226+
@Override
227+
public void securedUserOrAdmin() {
228+
super.securedUserOrAdmin();
229+
}
230+
231+
@Override
232+
public void inheritedAnnotations() {
233+
super.inheritedAnnotations();
234+
}
235+
236+
}
237+
195238
}

0 commit comments

Comments
 (0)