spring-projects / spring-graphql

Spring Integration for GraphQL
https://spring.io/projects/spring-graphql
Apache License 2.0
1.5k stars 297 forks source link

`@AuthenticationPrincipal` resolver should have precedence over `Principal` resolver #982

Closed makdim33 closed 2 weeks ago

makdim33 commented 1 month ago

Hello!

I'm using spring-graphql 1.2.6 in a spring-boot 3.1.11 app with spring-boot-starter-web (MVC implementation).

I have @Controller with a simple @QueryMapping that fetches data of the logged user.

@Controller
public class UserController {
    @QueryMapping
    public UserDto user(@AuthenticationPrincipal GraphQlPrincipal principal) {..}
}

My principal looks like :

import java.security.Principal;

public record GraphQlPrincipal(String name, String email) implements Principal {..}

Calling user query fails to correctly resolves the principal and results in an argument type mismatch error :

java.lang.IllegalStateException: argument type mismatch
Class [com.mypackage.graphql.controller.UserController]
Method [com.mypackage.graphql.dto.UserDto com.mypackage.graphql.controller.UserController.user(com.mypackage.security.principal.GraphQlPrincipal)] with argument values:
[0] [type=org.springframework.security.authentication.UsernamePasswordAuthenticationToken] 
[value=UsernamePasswordAuthenticationToken [Principal=GraphQlPrincipal[name=makdim, email=mohamed.akdim@mirakl.com], Credentials=[PROTECTED], Authenticated=true, Details=null, Granted Authorities=[USER]]] 
    at org.springframework.graphql.data.method.InvocableHandlerMethodSupport.doInvoke(InvocableHandlerMethodSupport.java:93)
    ...

According to documentation, this annotation should work right out of the box. The problem is that it resolves the Authentication object (which in my case is of type UsernamePasswordAuthenticationToken) instead of the actual Principal.

For instance, this works perfectly :

@QueryMapping
public UserDto user(@AuthenticationPrincipal UsernamePasswordAuthenticationToken authentication) {..}

After some investigations, it seems that the PrincipalMethodArgumentResolver is not working as expected : https://github.com/spring-projects/spring-graphql/blob/da473ca00bbfbf733a00cfbc1ef2b322f8a6bb9b/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/PrincipalMethodArgumentResolver.java#L56

The method resolveArgument() returns the resolved Authentication by resolveAuthentication() method and should instead returns the Principal (something like Authentication#getPrincipal())

PS : Since the AnnotatedControllerConfigurer registers the two resolvers PrincipalMethodArgumentResolver and AuthenticationPrincipalArgumentResolver in this order, and my GraphQlPrincipal implements java.security.Principal, the first resolver is picked and fails. Removing the implementation of java.security.Principal results in a successful resolution through the second resolver AuthenticationPrincipalArgumentResolver.

Thanks.

rstoyanchev commented 1 month ago

Yes it makes sense that @AuthenticationPrincipal should be checked first since it does something more specific, with the Principal resolver coming second as a fallback.