Netflix / dgs-framework

GraphQL for Java with Spring Boot made easy.
https://netflix.github.io/dgs
Apache License 2.0
3.09k stars 296 forks source link

bug: @PreAuthorize doesn't work with @DgsComponent #458

Open shiyouping opened 3 years ago

shiyouping commented 3 years ago

If the application is enabled by @EnableReactiveMethodSecurity, and a class is annotated by @DgsComponent with its method annotated by @PreAuthorize, there will be no Authentication found, which leads to an access denied issue (see details at https://github.com/Netflix/dgs-framework/discussions/381). But @PreAuthorize works well with @RestController.

Expected behavior

Authentication should be kept in ReactiveSecurityContextHolder

Actual behavior

Authentication is lost when PrePostAdviceReactiveMethodInterceptor tries to retrieve it in the method of invoke(final MethodInvocation invocation)

Steps to reproduce

  1. Enable Webflux method security

    @Configuration
    @EnableWebFluxSecurity
    @EnableReactiveMethodSecurity
    public class SecurityConfig {
    ...
    }
  2. Create a data fetcher annotated with DgsComponent

  3. Create a method annotated with PreAuthorize

    @DgsComponent
    public class UserDataFetcher {
    
    @DgsMutation
    @PreAuthorize("hasRole('ROLE_BUYER')")
    public Mono<String> signUp(@InputArgument("user") CreateUserModel createUserModel) {
    ...
    }
    }
  4. signUp method won't be accessible even the token is already set

shiyouping commented 3 years ago

When use @PreAuthorize together with @RestController, PrePostAdviceReactiveMethodInterceptor can get the Authorization from Context. So I guess DGS doesn't save the Authorization in the right Context before the protected methods are invoked.

hantsy commented 3 years ago

@shiyouping I think the @PreAuthorize works well with a @Component, check my Reactive Security example.

But I also encountered a similar issue in my DGS WebFlux sample project, check #555 .

(it always throws a Reflection related exception). The exception is thrown as expected, but finally, it is converted to an INTERNAL error type, not PERMISSION_DENIED in the Servlet stack.

As mentioned in the https://github.com/Netflix/dgs-framework/discussions/381, @Secures and jsr250 are not supported in WebFlux now.

ndwhelan commented 2 years ago

I'm having the same issue. I've spent an obscene amount of time sorting out Spring Security, and this is a real bummer.

When I debug in to hasRole, looking at the variable roleSet in SecurityExpressionRoot#hasAnyAuthorityName, the only role is ROLE_ANONYMOUS.

This is the annotation on my @DgsComponent method:

@DgsMutation
@PreAuthorize("hasRole('HAS_FEED_MAPPING')")

Yet, this is my security configuration:

return http
                .authorizeExchange().anyExchange().authenticated()
                .and()
                .httpBasic()
                .disable()
                .formLogin()
                .disable()
                .headers().frameOptions().disable()
                .and()
                .csrf().disable()
                .addFilterAt(authenticationWebFilter, SecurityWebFiltersOrder.AUTHENTICATION)
                .build();

That filter forces authentication. Though, regardless, this shows that I must be authenticated to hit the base page, despite EnableReactiveMethodSecurity not working correctly.

If I remove the @PreAuthorize annotation, and the addFilterAt(...) portion of my configuration, things don't get to checking the role, then nothing is authenticating me. This tells me that everything is correct right up to DGS.

srinivasankavitha commented 2 years ago

Thanks for reporting the bug, we haven't been able to look into this yet. We will post an update when we have more information on the what is causing this problem. Ideally, we would expect all the annotations to work seamlessly in conjunction with the framework.

On Thu, Sep 15, 2022 at 7:15 PM Nick Whelan @.***> wrote:

I'm having the same issue. I've spent an obscene amount of time sorting out Spring Security, and this is a real bummer.

When I debug in to hasRole, looking at the variable roleSet in SecurityExpressionRoot#hasAnyAuthorityName, the only role is ROLE_ANONYMOUS.

This is the annotation on my @DgsComponent method:

@@.***("hasRole('HAS_FEED_MAPPING')")

Yet, this is my security configuration:

return http .authorizeExchange().anyExchange().authenticated() .and() .httpBasic() .disable() .formLogin() .disable() .headers().frameOptions().disable() .and() .csrf().disable() .addFilterAt(authenticationWebFilter, SecurityWebFiltersOrder.AUTHENTICATION) .build();

That filter forces authentication. Though, regardless, this shows that I must be authenticated to hit the base page, despite EnableReactiveMethodSecurity not working correctly.

If I remove the @PreAuthorize annotation, and the addFilterAt(...) portion of my configuration, things don't get to checking the role, then nothing is authenticating me. This tells me that everything is correct right up to DGS.

— Reply to this email directly, view it on GitHub https://github.com/Netflix/dgs-framework/issues/458#issuecomment-1248834906, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJ5JPXN2TSTFK4ZLJFEZFLDV6PJ55ANCNFSM47T3OBQA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

ndwhelan commented 2 years ago

I wanted to provide a little more information.

I think there may be two things that need to be resolved.

First, the granted authorities need to get propagated, such that they exist for @PreAuthorize annotations on DGS component methods.

Secondly, org.springframework.security.access.AccessDeniedException doesn't appear to be handled correctly. When using @PreAuthorize, currently, this is thrown every time. It results in a generic InternalServerError, instead of a forbidden error. I would expect errors[*].extensions.code to be FORBIDDEN instead of INTERNAL_SERVER_ERROR.

In addition to that information, I wanted to share that the following results in full denial to /graphql, but that's not really what I need to have. It gives additional support that this is something specific to method level security, DGS, and WebFlux.

return http
            .authorizeExchange().pathMatchers("/graphql").hasRole("HAS_FEED_MAPPING")
            .and()
            .httpBasic()
            .disable()
            .formLogin()
            .disable()
            .headers().frameOptions().disable()
            .and()
            .csrf().disable()
            .addFilterAt(authFilter, SecurityWebFiltersOrder.AUTHENTICATION)
            .build();

Thanks in advance for any investigation or fixes :-)

srinivasankavitha commented 2 years ago

Great, thanks so much for the additional context!

antholeole commented 1 year ago

This bug is invalid. Not sure what the OP is doing incorrect (will need more context), but the following works perfectly fine:

@Configuration
@EnableWebFluxSecurity
@EnableReactiveMethodSecurity
public class SecurityConfig {
    @Bean
    public SecurityWebFilterChain springSecurityFilterChain(ServerHttpSecurity http) {
        return http
                .addFilterAt(new ExceptionTranslationWebFilter(), SecurityWebFiltersOrder.EXCEPTION_TRANSLATION)
                .httpBasic()
                .and()
                .csrf()
                .disable()
                .build();
    }

    @Bean
    public MapReactiveUserDetailsService userDetailsService() {
        User.UserBuilder userBuilder = User.withDefaultPasswordEncoder();
        UserDetails rob = userBuilder.username("rob")
                .password("rob")
                .roles("USER")
                .build();
        UserDetails admin = userBuilder.username("admin")
                .password("admin")
                .roles("USER","ADMIN")
                .build();
        return new MapReactiveUserDetailsService(rob, admin);
    }
}
@DgsComponent
public class ReactiveDataFetchers {
    @DgsQuery
    @PreAuthorize("hasRole('ADMIN')")
    public Mono<String> mono() {
        return Mono.just("hello mono");
    }

    @DgsQuery
    public Flux<Integer> flux() {
        return Flux.just(1, 2, 3);
    }
}

Then, sending a graphql request to mono authenticated responds with:

{"data":{"mono":"hello mono"}}

as expected. Sending a query unauthenticated responds with a 401 unauthorized, also as expected. This is standard spring security - we do nothing DGS specific. I don't believe a doc update is required.

also @ndwhelan, take note of .addFilterAt(new ExceptionTranslationWebFilter(), SecurityWebFiltersOrder.EXCEPTION_TRANSLATION) being applied to the root, and not /graphql: when using DGS resolvers, there is no path associated with them and thus the ExceptionTranslationWebFilter does not apply, meaning you get a 500 instead of a 401. Add that filter to your root and your exception should go away!