From fcca5f5e64a94220b1db0da2068a17cc71df9d7f Mon Sep 17 00:00:00 2001 From: Anthony Raymond Date: Wed, 16 Sep 2020 15:26:34 +0200 Subject: [PATCH 1/4] Disable ArgumentResolver for annotated Principal ServletRequestMethodArgumentResolver happend early in the `ArgumentResolver` chain. One of this ability is to resolve `Principal`, that's great and it works well. But most of the time when a parametre of type `Principal` is annotated we don't want to get the `Principal` from the `HttpServletRequest.getUserPrincipal()`. This feature is actually conflicting with the **spring-security** documentation and the `@AuthenticationPrincipal` annotation which is supposed to resolve the Principal from Authentication.getPrincipal(). Fix #4151 --- .../annotation/ServletRequestMethodArgumentResolver.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ServletRequestMethodArgumentResolver.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ServletRequestMethodArgumentResolver.java index f1f5baf728a3..18b777bcb71f 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ServletRequestMethodArgumentResolver.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ServletRequestMethodArgumentResolver.java @@ -84,12 +84,13 @@ public class ServletRequestMethodArgumentResolver implements HandlerMethodArgume @Override public boolean supportsParameter(MethodParameter parameter) { Class paramType = parameter.getParameterType(); + final Annotation[] parameterAnnotations = parameter.getParameterAnnotations(); return (WebRequest.class.isAssignableFrom(paramType) || ServletRequest.class.isAssignableFrom(paramType) || MultipartRequest.class.isAssignableFrom(paramType) || HttpSession.class.isAssignableFrom(paramType) || (pushBuilder != null && pushBuilder.isAssignableFrom(paramType)) || - Principal.class.isAssignableFrom(paramType) || + (Principal.class.isAssignableFrom(paramType) && parameterAnnotations.length == 0) || InputStream.class.isAssignableFrom(paramType) || Reader.class.isAssignableFrom(paramType) || HttpMethod.class == paramType || From dec530ab366c7f571dc726c6998fe02911f71c4c Mon Sep 17 00:00:00 2001 From: Anthony Raymond Date: Tue, 22 Sep 2020 13:58:49 +0200 Subject: [PATCH 2/4] Add missing import --- .../method/annotation/ServletRequestMethodArgumentResolver.java | 1 + 1 file changed, 1 insertion(+) diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ServletRequestMethodArgumentResolver.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ServletRequestMethodArgumentResolver.java index 18b777bcb71f..36ac97637b55 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ServletRequestMethodArgumentResolver.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ServletRequestMethodArgumentResolver.java @@ -19,6 +19,7 @@ import java.io.IOException; import java.io.InputStream; import java.io.Reader; +import java.lang.annotation.Annotation; import java.security.Principal; import java.time.ZoneId; import java.util.Locale; From b014ff5cf7aa66fc8496b63be13eb82e6852b6c5 Mon Sep 17 00:00:00 2001 From: Anthony Raymond Date: Tue, 22 Sep 2020 13:59:54 +0200 Subject: [PATCH 3/4] Add test to ensure non-regression --- ...letRequestMethodArgumentResolverTests.java | 49 ++++++++++++++----- 1 file changed, 36 insertions(+), 13 deletions(-) diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletRequestMethodArgumentResolverTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletRequestMethodArgumentResolverTests.java index cddb9624d0dd..0a90f97490c5 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletRequestMethodArgumentResolverTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletRequestMethodArgumentResolverTests.java @@ -16,22 +16,9 @@ package org.springframework.web.servlet.mvc.method.annotation; -import java.io.InputStream; -import java.io.Reader; -import java.lang.reflect.Method; -import java.security.Principal; -import java.time.ZoneId; -import java.util.Locale; -import java.util.TimeZone; - -import javax.servlet.ServletRequest; -import javax.servlet.http.HttpSession; -import javax.servlet.http.PushBuilder; - import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.mockito.Mockito; - import org.springframework.core.MethodParameter; import org.springframework.http.HttpMethod; import org.springframework.web.context.request.ServletWebRequest; @@ -44,6 +31,21 @@ import org.springframework.web.testfixture.servlet.MockHttpServletResponse; import org.springframework.web.testfixture.servlet.MockHttpSession; +import javax.servlet.ServletRequest; +import javax.servlet.http.HttpSession; +import javax.servlet.http.PushBuilder; +import java.io.InputStream; +import java.io.Reader; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; +import java.lang.reflect.Method; +import java.security.Principal; +import java.time.ZoneId; +import java.util.Locale; +import java.util.TimeZone; + import static org.assertj.core.api.Assertions.assertThat; /** @@ -122,6 +124,19 @@ public void principalAsNull() throws Exception { assertThat(result).as("Invalid result").isNull(); } + // spring-security already provides the @AuthenticationPrincipal annotation to inject the Principal taken from SecurityContext.getAuthentication.getPrincipal() + // but ServletRequestMethodArgumentResolver used to take precedence over @AuthenticationPrincipal resolver org.springframework.security.web.method.annotation.AuthenticationPrincipalArgumentResolver + // and we used to get the wrong Principal in methods. See https://github.com/spring-projects/spring-framework/pull/25780 + @Test + public void annotatedPrincipal() throws Exception { + Principal principal = () -> "Foo"; + servletRequest.setUserPrincipal(principal); + Method principalMethod = getClass().getMethod("supportedParamsWithAnnotatedPrincipal", Principal.class); + + MethodParameter principalParameter = new MethodParameter(principalMethod, 0); + assertThat(resolver.supportsParameter(principalParameter)).as("Principal not supported").isFalse(); + } + @Test public void locale() throws Exception { Locale locale = Locale.ENGLISH; @@ -245,6 +260,14 @@ public PushBuilder newPushBuilder() { assertThat(result).as("Invalid result").isSameAs(pushBuilder); } + @Target({ ElementType.PARAMETER }) + @Retention(RetentionPolicy.RUNTIME) + public @interface PlaceHolder {} + + @SuppressWarnings("unused") + public void supportedParamsWithAnnotatedPrincipal(@PlaceHolder Principal p) { + + } @SuppressWarnings("unused") public void supportedParams(ServletRequest p0, From 2dfebc0693654936a29cd118b384ce4625a292df Mon Sep 17 00:00:00 2001 From: Anthony Raymond Date: Tue, 22 Sep 2020 17:11:55 +0200 Subject: [PATCH 4/4] Restore imports order according to project guidelines for imports --- ...letRequestMethodArgumentResolverTests.java | 32 ++++++++++--------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletRequestMethodArgumentResolverTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletRequestMethodArgumentResolverTests.java index 0a90f97490c5..ee8f131cb34d 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletRequestMethodArgumentResolverTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletRequestMethodArgumentResolverTests.java @@ -16,9 +16,26 @@ package org.springframework.web.servlet.mvc.method.annotation; +import java.io.InputStream; +import java.io.Reader; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; +import java.lang.reflect.Method; +import java.security.Principal; +import java.time.ZoneId; +import java.util.Locale; +import java.util.TimeZone; + +import javax.servlet.ServletRequest; +import javax.servlet.http.HttpSession; +import javax.servlet.http.PushBuilder; + import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.mockito.Mockito; + import org.springframework.core.MethodParameter; import org.springframework.http.HttpMethod; import org.springframework.web.context.request.ServletWebRequest; @@ -31,21 +48,6 @@ import org.springframework.web.testfixture.servlet.MockHttpServletResponse; import org.springframework.web.testfixture.servlet.MockHttpSession; -import javax.servlet.ServletRequest; -import javax.servlet.http.HttpSession; -import javax.servlet.http.PushBuilder; -import java.io.InputStream; -import java.io.Reader; -import java.lang.annotation.ElementType; -import java.lang.annotation.Retention; -import java.lang.annotation.RetentionPolicy; -import java.lang.annotation.Target; -import java.lang.reflect.Method; -import java.security.Principal; -import java.time.ZoneId; -import java.util.Locale; -import java.util.TimeZone; - import static org.assertj.core.api.Assertions.assertThat; /**