Resolve Principal argument only when not annotated #25780

anthonyraymond · 2020-09-16T13:31:22Z

ServletRequestMethodArgumentResolver happend early in the ArgumentResolver chain. One of his ability is to resolve Principal, that's great and it works well.

But when a parameter of type Principal is annotated we don't want to get the Principal from the HttpServletRequest.getUserPrincipal().
This feature is even in conflict with the spring-security documentation and the @AuthenticationPrincipal annotation which is supposed to resolve the Principal from Authentication.getPrincipal().

Having the ServletRequestMethodArgumentResolver resolving annotated Principal makes the @AuthenticationPrincipal and AuthenticationPrincipalArgumentResolver useless and missleading.

Fix spring-projects/spring-security#4151

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

pivotal-issuemaster · 2020-09-16T13:31:24Z

@anthonyraymond Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

pivotal-issuemaster · 2020-09-16T13:32:07Z

@anthonyraymond Thank you for signing the Contributor License Agreement!

sbrannen · 2020-09-17T09:35:51Z

.../springframework/web/servlet/mvc/method/annotation/ServletRequestMethodArgumentResolver.java

 		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) ||


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.

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?

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?

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 .

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.

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.

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.

imjmakki

great job

rstoyanchev · 2020-09-22T10:07:20Z

Thanks for the extra detail. This is scheduled for 5.3 now.

Why is the spring-framework trying to inject a Principal ?

It is one of many request properties that ServletRequestMethodArgumentResolver aims to expose.

anthonyraymond · 2020-09-22T12:06:04Z

Thanks for the quick reply.

Since this will most likely get merged, i added a test to ensure it wont break in the future.

...ngframework/web/servlet/mvc/method/annotation/ServletRequestMethodArgumentResolverTests.java

imjmakki

nice

Closes gh-25780

imjmakki

good

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Sep 16, 2020

sbrannen added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Sep 16, 2020

sbrannen reviewed Sep 17, 2020

View reviewed changes

sbrannen mentioned this pull request Sep 17, 2020

AuthenticationPrincipalArgumentResolver not used when argument class implements Principal spring-projects/spring-security#4151

Closed

imjmakki reviewed Sep 20, 2020

View reviewed changes

rstoyanchev added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Sep 22, 2020

rstoyanchev self-assigned this Sep 22, 2020

rstoyanchev added this to the 5.3 RC2 milestone Sep 22, 2020

rstoyanchev changed the title ~~Disable ArgumentResolver for annotated Principal~~ Resolve Principal argument only when not annotated Sep 22, 2020

anthonyraymond added 2 commits September 22, 2020 14:03

Add missing import

dec530a

Add test to ensure non-regression

b014ff5

sbrannen reviewed Sep 22, 2020

View reviewed changes

...ngframework/web/servlet/mvc/method/annotation/ServletRequestMethodArgumentResolverTests.java Outdated Show resolved Hide resolved

Restore imports order according to project guidelines for imports

2dfebc0

imjmakki reviewed Sep 24, 2020

View reviewed changes

rstoyanchev closed this in 10eb5bd Sep 25, 2020

rstoyanchev added a commit that referenced this pull request Sep 25, 2020

Polishing contribution

c45be0b

Closes gh-25780

imjmakki reviewed Sep 26, 2020

View reviewed changes

sbrannen mentioned this pull request Oct 28, 2020

@Nonnull annotation breaks ServletRequestMethodArgumentResolver for Principal #25981

Closed

petergphillips mentioned this pull request Nov 19, 2020

Spring Boot 2.4 upgrade breaks injection of Principal #26117

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Resolve Principal argument only when not annotated #25780

Resolve Principal argument only when not annotated #25780

Uh oh!

anthonyraymond commented Sep 16, 2020 •

edited

Loading

Uh oh!

pivotal-issuemaster commented Sep 16, 2020

Uh oh!

pivotal-issuemaster commented Sep 16, 2020

Uh oh!

sbrannen Sep 17, 2020

Uh oh!

sbrannen Sep 17, 2020 •

edited

Loading

Uh oh!

rstoyanchev Sep 21, 2020

Uh oh!

rstoyanchev Sep 21, 2020 •

edited

Loading

Uh oh!

sbrannen Sep 21, 2020

Uh oh!

sbrannen Sep 21, 2020

Uh oh!

anthonyraymond Sep 22, 2020 •

edited

Loading

Uh oh!

imjmakki left a comment

Uh oh!

rstoyanchev commented Sep 22, 2020

Uh oh!

anthonyraymond commented Sep 22, 2020

Uh oh!

Uh oh!

imjmakki left a comment

Uh oh!

imjmakki left a comment

Uh oh!

Uh oh!

Resolve Principal argument only when not annotated #25780

Resolve Principal argument only when not annotated #25780

Uh oh!

Conversation

anthonyraymond commented Sep 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pivotal-issuemaster commented Sep 16, 2020

Uh oh!

pivotal-issuemaster commented Sep 16, 2020

Uh oh!

sbrannen Sep 17, 2020

Choose a reason for hiding this comment

Uh oh!

sbrannen Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rstoyanchev Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

rstoyanchev Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbrannen Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

sbrannen Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

anthonyraymond Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imjmakki left a comment

Choose a reason for hiding this comment

Uh oh!

rstoyanchev commented Sep 22, 2020

Uh oh!

anthonyraymond commented Sep 22, 2020

Uh oh!

Uh oh!

imjmakki left a comment

Choose a reason for hiding this comment

Uh oh!

imjmakki left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

anthonyraymond commented Sep 16, 2020 •

edited

Loading

sbrannen Sep 17, 2020 •

edited

Loading

rstoyanchev Sep 21, 2020 •

edited

Loading

anthonyraymond Sep 22, 2020 •

edited

Loading