Skip to content

Resolve Principal argument only when not annotated #25780

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

Resolve Principal argument only when not annotated #25780

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fcca5f5
Disable ArgumentResolver for annotated Principal
anthonyraymond Sep 16, 2020
dec530a
Add missing import
anthonyraymond Sep 22, 2020
b014ff5
Add test to ensure non-regression
anthonyraymond Sep 22, 2020
2dfebc0
Restore imports order according to project guidelines for imports
anthonyraymond Sep 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion ...ringframework/web/servlet/mvc/method/annotation/ServletRequestMethodArgumentResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -84,12 +85,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) ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merely checking for the absence of an annotation would not be sufficient, since the parameter could be annotated with annotations other than Spring Security's @AuthenticationPrincipal.

Copy link
Member

@sbrannen sbrannen Sep 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the Spring Security issue would need to be addressed within Spring Security, potentially making use of RequestMappingHandlerAdapter.setArgumentResolvers(List<HandlerMethodArgumentResolver>) (as opposed to setCustomArgumentResolvers(...)).

@rstoyanchev and @rwinch, thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think setting the default resolvers is a good way to go.

We have some precedent with arguments of type Map where we do check that it isn't annotated in some way, see 5a3ff35. We could so something similar here given that Principal is also a very high level interface with many possible uses and implementations. What I'm wondering though is why this problem surfaces only now, was there some change in Spring Security or did it never work?

Copy link
Contributor

@rstoyanchev rstoyanchev Sep 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or does it depend on the user Object of the application?

Under spring-projects/spring-security#4151 it says one option is for UserDetails to not inherit Principal but from what I see UserDetails doesn't .

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have some precedent with arguments of type Map where we do check that it isn't annotated in some way, see 5a3ff35.

OK. I wasn't aware of that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or does it depend on the user Object of the application?

Yes, it depends on the user-specific implementation of UserDetails.

Under spring-projects/spring-security#4151 it says one option is for UserDetails to not inherit Principal but from what I see UserDetails doesn't .

Right. The UserDetails interface does not extend Principal by default, though a developer may of course choose to implement Principal if they want (or need) to.

I have implemented in UserDetails in several applications of the years, and I have never had it implement Principal as well. I also have never seen anyone do that. But... as I mentioned above, it's certainly an option if it makes sense in a given application.

Copy link
Contributor Author

@anthonyraymond anthonyraymond Sep 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi guys thanks for your investment in this issue.

to answer @rstoyanchev

What I'm wondering though is why this problem surfaces only now, was there some change in Spring Security or did it never work?

My guess is that it's never worked, people use the @AuthenticationPrincipal and they get what they asks for: a Principal. It's not the expected one, because it's not coming from the right place, but it's a principal and it's good enough after 3 hours of tweaking the Auth process to get what you want🤝. All Most peoples HATES having to deal with auth 😨, and if the framework is handling this good enough that's a deal 😄 .

But this behavior has always been a problem:

  • 2019 SO post: HttpServletRequest.getUserPrincipal() contains the Authentication (which extends Principal) so it returns it, and it does not extract the Principal from the Authentication. There are multiple solutions for this around SO and most of them say : Use org.keycloak.adapters.springsecurity.token.KeycloakAuthenticationToken in place of KeycloakPrincipal (which is an implementation of Authentication) and magically it starts to work, but not for the good reasons.
  • 2015 SO post: Because HttpServletRequest.getUserPrincipal() contains the Authentication (which extends Principal).

Basically what you can get with the Google search "Current user principal is not of type" is related to this issue.

Both of these issues come from the fact that the Principal resolved is from HttpServletRequest.getUserPrincipal() instead of SecurityContext.getAuthentication.getPrincipal().

The nasty thing about that is that the @AuthenticationPrincipal has not only become not relevant but also missleading, because all class extending Principal can be injected into a Principal type (thanks to the spring-framework, the annotation takes no part in this), but as soon as you want to get your implementation of Principal you are doomed.


A side question that i can't figure out on my own (no sarcasm, that's a real question):

  • Why is the spring-framework trying to inject a Principal ? Isn't java.security.Principal informaly but well accepted as an authentication class? IMHO it makes more sense to delegate that behaviour to the security part of spring.

InputStream.class.isAssignableFrom(paramType) ||
Reader.class.isAssignableFrom(paramType) ||
HttpMethod.class == paramType ||
Expand Down
25 changes: 25 additions & 0 deletions ...ramework/web/servlet/mvc/method/annotation/ServletRequestMethodArgumentResolverTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@

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;
Expand Down Expand Up @@ -122,6 +126,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;
Expand Down Expand Up @@ -245,6 +262,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,
Expand Down