Skip to content
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.

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

Spring Boot 2.4 upgrade breaks injection of Principal #26117

Closed
petergphillips opened this issue Nov 19, 2020 · 23 comments
Closed

Spring Boot 2.4 upgrade breaks injection of Principal #26117

petergphillips opened this issue Nov 19, 2020 · 23 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug
Milestone

Comments

@petergphillips
Copy link

Affects: 2.4.0

Our code:

  fun me(@ApiIgnore principal: Principal): UserDetail {

used to inject the principal in Spring Boot versions prior to 2.4.0. Under 2.4.0 the argument is then null and our application breaks.

If the @ApiIgnore annotation is removed then the principal is then injected, however we don't want the principal to be exposed in our API documentation since it is an internal parameter.

It appears that the bug was introduced in #25780. That PR doesn't check to see if there is a AuthenticationPrincipal annotation on the field, merely that the parameter has any annotations at all so even Nonnull will break the injection.

We've tried adding the AuthenticationPrincipal annotation on the field, however that doesn't work since the parameter resolver tries to inject authentication.getPrincipal which in our case is a String since we're using spring security oauth2. We want the Principal injected instead.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Nov 19, 2020
@electrified
Copy link

I have found a resolution to the issue by adding the following:

@Configuration
class WebConfig : WebMvcConfigurer {
  override fun addArgumentResolvers(resolvers: MutableList<HandlerMethodArgumentResolver>) {
    super.addArgumentResolvers(resolvers)

    resolvers.add(PrincipalMethodArgumentResolver())
  }
}

The new PrincipalMethodArgumentResolver is in the defaultInitBinderArgumentResolvers but not the defaultArgumentResolvers https://github.com/spring-projects/spring-framework/blob/master/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerAdapter.java#L718

@rstoyanchev rstoyanchev self-assigned this Nov 19, 2020
@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Nov 19, 2020
@rstoyanchev rstoyanchev added this to the 5.3.2 milestone Nov 19, 2020
@rstoyanchev
Copy link
Contributor

Yes it looks like the fix in #25981 was incomplete.

@michaelbrewer
Copy link

@rstoyanchev i am still experiencing this issue. Is the fix @electrified recommending required for this. Or is there another reason why this is closed?

@michaelbrewer
Copy link

michaelbrewer commented Dec 15, 2020

Same issue as @petergphillips

    fun createPayment(
        request: HttpServletRequest,
        @ApiIgnore @AuthenticationPrincipal authentication: JwtAuthentication<PaymentGatewayClaim>,
        @RequestBody @Valid paymentCreateRequest: PaymentCreateRequest
    ): PaymentCreateResponse {

Adding this did fix this, but i don't understand why this was changed?

@Configuration
class WebConfig : WebMvcConfigurer {
  override fun addArgumentResolvers(resolvers: MutableList<HandlerMethodArgumentResolver>) {
    super.addArgumentResolvers(resolvers)

    resolvers.add(PrincipalMethodArgumentResolver())
  }
}

@rstoyanchev
Copy link
Contributor

@michaelbrewer if you look at the timeline you will see that it was closed with a commit. That commit adds the same resolver and there are tests for it. Can you check that you are running with the fix from that commit, which should be included in version 5.3.2?

@michaelbrewer
Copy link

@rstoyanchev i am still experiencing this with SpringBoot 2.4.1 (maybe i will have to manually bump spring-framework to 5.3.2?)

@michaelbrewer
Copy link

@rstoyanchev So i have to use the PrincipalMethodArgumentResolver to fix this regression by Spring? SpringBoot 2.4.1 is using Spring Framework 5.3.2 Upgrade to Spring Framework 5.3.2 #24278

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Dec 17, 2020

@michaelbrewer please check the fix 05e3f27. It does exactly the same. If you can double check in the source that this change is present. Also maybe debug to see what happens? I don't see why you have to add this resolver if it is there.

@michaelbrewer
Copy link

@rstoyanchev still does not work for me. But i am using manual @Import and not using MVC. Is there a specific configuration this works for? (I do won't why regression bugs like this are needed.)

@fletchgqc
Copy link

fletchgqc commented Jan 4, 2021

@rstoyanchev I see your point, that the fix does indeed add the resolver. Nevertheless, I can confirm that I also suffer this problem under Spring Boot 2.4.1, and I have confirmed that the fix is visible in the code.

When I add the following (Java version of @electrified's fix), then the problem disappears.

@Configuration
public class Something implements WebMvcConfigurer {

    @Override
    public void addArgumentResolvers(List<HandlerMethodArgumentResolver> resolvers) {
            resolvers.add(new PrincipalMethodArgumentResolver());
    }
}

Is the order of the resolvers relevant? I debugged a bit and with the @Configuration fix, the PrincipalMethodArgumentResolver is now added twice, the @Configuration class causes it to be added in spot 25 in the list for my example, along with being added to spot 33 by the RequestMappingHandlerAdapter.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jan 7, 2021

@michaelbrewer, I'm not sure what manual @Import vs not using MVC is exactly or why that would have an impact since the PrincipalMethodArgumentResolver is inserted inside RequestMappingHandlerAdapter.

@fletchgqc a search on usages for where the resolver is instantiated does not explain why it would be added twice. Could it be that it's twice because you're adding it as well? The framework adds it once only.

Can either of you provide a sample that demonstrates the issue?

@fletchgqc
Copy link

fletchgqc commented Jan 8, 2021

@rstoyanchev It's added twice, because I use Spring 2.4.1 (which has your fix), and also the fix mentioned at the top of this thread. With plain Spring 2.4.1 it's only added once, but that doesn't fix the problem.

@petergphillips
Copy link
Author

Our issue is fixed in Spring 2.4.1. We've removed @electrified fix and the application still works as expected 😁

@krm1312
Copy link

krm1312 commented Jan 12, 2021

Attempted upgrade from boot 2.3.4 to 2.4.1 and @AuthenticationPrincipal is now null in our relatively large application as well. Will try to debug a bit.

@rstoyanchev
Copy link
Contributor

@krm1312 thanks and if there is a regression please create a separate issue to provide the details.

@michaelbrewer
Copy link

@rstoyanchev Still not fixed for me.

Following code works fine on SpringBoot 2.3.8 but on SpringBoot 2.4.2 the auth parameter is null:

java.lang.NullPointerException: Parameter specified as non-null is null: method temp.FakeController.fake, parameter auth
	at temp.FakeController.fake(AppTest.kt)
package temp

import okhttp3.OkHttpClient
import okhttp3.Request
import org.junit.jupiter.api.Assertions
import org.junit.jupiter.api.Test
import org.junit.jupiter.api.TestInstance
import org.junit.jupiter.api.extension.ExtendWith
import org.springframework.boot.autoconfigure.SpringBootApplication
import org.springframework.boot.test.context.SpringBootTest
import org.springframework.boot.web.server.LocalServerPort
import org.springframework.context.annotation.Configuration
import org.springframework.security.authentication.UsernamePasswordAuthenticationToken
import org.springframework.security.config.annotation.web.builders.HttpSecurity
import org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter
import org.springframework.security.core.Authentication
import org.springframework.security.core.annotation.AuthenticationPrincipal
import org.springframework.security.core.context.SecurityContextHolder
import org.springframework.security.web.authentication.www.BasicAuthenticationFilter
import org.springframework.test.context.ContextConfiguration
import org.springframework.test.context.junit.jupiter.SpringExtension
import org.springframework.web.bind.annotation.GetMapping
import org.springframework.web.bind.annotation.RestController
import org.springframework.web.filter.GenericFilterBean
import javax.servlet.FilterChain
import javax.servlet.ServletRequest
import javax.servlet.ServletResponse

@SpringBootApplication
class FakeApplication

class FakeSecurityFilter : GenericFilterBean() {
    override fun doFilter(request: ServletRequest, response: ServletResponse, chain: FilterChain) {
        SecurityContextHolder.getContext().authentication = UsernamePasswordAuthenticationToken("x", "y")
        chain.doFilter(request, response)
    }
}

@Configuration
class CustomWebSecurityConfigurerAdapter : WebSecurityConfigurerAdapter() {
    override fun configure(http: HttpSecurity) {
        http.addFilterAfter(FakeSecurityFilter(), BasicAuthenticationFilter::class.java)
    }
}

@RestController
class FakeController {
    @GetMapping(path = ["/fake"])
    fun fake(@AuthenticationPrincipal auth: Authentication) {
        println(auth)
    }
}

@ExtendWith(SpringExtension::class)
@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT)
@ContextConfiguration(classes = [FakeApplication::class])
@TestInstance(TestInstance.Lifecycle.PER_CLASS)
class Tests {
    @LocalServerPort
    private val port = 0

    @Test
    fun checkCall() {
        val request = Request.Builder().get().url("http://localhost:$port/fake").build()
        val response = OkHttpClient().newCall(request).execute()
        Assertions.assertEquals(200, response.code)
    }
}

@rstoyanchev
Copy link
Contributor

@michaelbrewer sounds like a duplicate of #26380.

@michaelbrewer
Copy link

Ok thanks so you are not supposed to use @AuthenticationPrincipal ?

@michaelbrewer
Copy link

So removing the @AuthenticationPrincipal works in the controller example

OR putting in this.

@Configuration
class WebConfig : WebMvcConfigurer {
    override fun addArgumentResolvers(resolvers: MutableList<HandlerMethodArgumentResolver>) {
        super.addArgumentResolvers(resolvers)
        resolvers.add(PrincipalMethodArgumentResolver())
    }
}

@rstoyanchev
Copy link
Contributor

so you are not supposed to use @AuthenticationPrincipal ?

No, it's not that you are not supposed but that it doesn't provide what you are trying to get. Your argument is Authentication but as per the Javadoc the annotation provides access to Authentication#getPrincipal(). The former is Principal, the latter is not.

@michaelbrewer
Copy link

so you are not supposed to use @AuthenticationPrincipal ?

No, it's not that you are not supposed but that it doesn't provide what you are trying to get. Your argument is Authentication but as per the Javadoc the annotation provides access to Authentication#getPrincipal(). The former is Principal, the latter is not.

Thanks for the clarification, interesting that adding PrincipalMethodArgumentResolver allows for this “bug” to continue to work.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jan 19, 2021

It is a bit confusing because the word "principal" is used in multiple contexts but there is no bug I think.

  1. Authentication is the top-level container class from Spring Security that implements Principal. It can be injected into a controller method without any annotations because it is accessible via HttpServletRequest#getUserPrincipal.
  2. Authentication#getPrincipal() exposes the user identity which as explained in the Javadoc can be as simple as a String but more likely a UserDetails or some other custom user object that is created by the AuthenticationManager.

@AuthenticationPrincipal provides a shortcut for 2), i.e. to the UserDetails or the some custom user object, without having to go through Spring Security's Authentication. This object is typically not a Principal but it could be.

Therefore when injecting with the annotation, it is important to use the correct user type. At the same time it is perfectly legitimate to inject Authentication or Principal if what you want is 1).

@rstoyanchev
Copy link
Contributor

In addition to the above change, the wiki has also been updated.

erinreeves pushed a commit to cedardevs/onestop that referenced this issue Feb 3, 2022
…al, since an old bug got fixed and changed the behavior of which got populated first spring-projects/spring-framework#26117
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug
Projects
None yet
Development

No branches or pull requests

7 participants