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

@AuthenticationPrincipal resolver should have precedence over Principal resolver #982

Closed
makdim33 opened this issue May 22, 2024 · 1 comment
Assignees
Labels
status: backported An issue that has been backported to maintenance branches type: bug A general bug
Milestone

Comments

@makdim33
Copy link

makdim33 commented May 22, 2024

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 protected]], 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 :

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.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 22, 2024
@rstoyanchev rstoyanchev added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 5, 2024
@rstoyanchev rstoyanchev changed the title @AuthenticationPrincipal not injecting the Principal as expected @AuthenticationPrincipal resolver should have precedence over Principal resolver Jun 5, 2024
@rstoyanchev
Copy link
Contributor

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.

@rstoyanchev rstoyanchev added this to the 1.3.1 milestone Jun 5, 2024
@rstoyanchev rstoyanchev added the for: backport-to-1.2.x Marks an issue as a candidate for backport to 1.2.x label Jun 5, 2024
@github-actions github-actions bot added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-1.2.x Marks an issue as a candidate for backport to 1.2.x labels Jun 5, 2024
@rstoyanchev rstoyanchev self-assigned this Jun 5, 2024
rstoyanchev added a commit that referenced this issue Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: backported An issue that has been backported to maintenance branches type: bug A general bug
Projects
None yet
Development

No branches or pull requests

3 participants