bug: Security Context from Spring Security doesn't get inherited by all threads

[luismolina13]

If the application is enabled by @EnableGlobalMethodSecurity(securedEnabled = true, prePostEnabled = true), and a method is annotated by @DgsData and @Secured("ROLE_ADMIN") in some cases the SecurityContextHolder will have a null authentication. We want to be able to block access to some data fields depending on the role of the user authenticated.

Expected behavior

Authentication from the SecurityContextHolder should be kept for all the threads decorating the data.

Actual behavior

My guess is that the SecurityContext is not being delegated to threads created from the main thread serving a request. See Spring Security Concurrency Support

Steps to reproduce

1. Enable method security

   @EnableWebSecurity
   @EnableGlobalMethodSecurity(securedEnabled = true, prePostEnabled = true)
   class WebConfig @Autowired constructor() : WebSecurityConfigurerAdapter() {
   ...
   }

2. Create a data fetcher annotated with @DgsComponent

3. Create a method annotated with @Secured

   @DgsComponent
   public class UserDataFetcher {
   
   @Secured
   @DgsData(parentType = DgsConstants.USER.TYPE_NAME)
   fun name(): String? {
       return "test"
   }}
   }

4. Make a call that tries to fetch many Users and decorate the name for all of them, some threads will be spawned that will not have the security context.

[luismolina13]

A work around we are currently looking into is to add SecurityContextHolder.setStrategyName(SecurityContextHolder.MODE_INHERITABLETHREADLOCAL) when configuring the application.

This seems to fix the issue, but it also feels like it could have unintended behavior or leak a security context to another thread?

[luismolina13]

Don't do this SecurityContextHolder.setStrategyName(SecurityContextHolder.MODE_INHERITABLETHREADLOCAL) testing locally we realized it indeed leaks the security context between threads.

We have been struggling to find a workaround, any help would be greatly appreciated.

Digging deeper it seems that the issue is with graphql-java using the ForkJoinPool.commonPool when running data fetchers asynchronously. graphql-spring seems to solve it using context propagation but I don't think this framework is doing it. https://docs.spring.io/spring-graphql/docs/current/reference/html/#execution.context

[srinivasankavitha]

In order to propagate context, please try supplying a task executor for async operations so the context gets propagated.: https://netflix.github.io/dgs/data-loaders/#using-spring-features-such-as-securitycontextholder-inside-a-completablefuture

[luismolina13]

hi @srinivasankavitha thanks for the pointer! It doesn't work though, by the time it reaches this point:

@DgsData(parentType = "Query", field = "list_things")
public CompletableFuture<List<Thing>> resolve(DataFetchingEnvironment environment) {
  // Security context is not available here anymore.
  return CompletableFuture.supplyAsync(() -> {
     return myService.getThings();
}, executor);

It is already running within the thread from ForkJoinPool.commonPool from graphql-java which doesn't have the context anymore.

[srinivasankavitha]

Are you also using this? https://docs.spring.io/spring-security/site/docs/5.0.x/reference/html/concurrency.html#delegatingsecuritycontextexecutor

The DGS framework does not do any explicit handling for propagating thread local context. Hence the user needs to ensure that. For our internal use cases, using a task executor that handles propagation of thread local context works well.

[luismolina13]

This is how I have tried doing it, but I may be doing something wrong:

@Configuration
@EnableWebMvc
@EnableWebSecurity
@EnableGlobalMethodSecurity(securedEnabled = true, prePostEnabled = true)
class WebConfig : WebMvcConfigurer {

  @Bean
  fun securityFilterChain(http: HttpSecurity): SecurityFilterChain {
    // ... Security configuration to set up Spring Security
  }

  @Bean
  fun securityContextExecutor(): Executor {
    val delegateExecutor = SimpleAsyncTaskExecutor()
    return DelegatingSecurityContextExecutor(delegateExecutor)
  }
}

And then in our data fetcher we have:

@DgsComponent
public class UserDataFetcher {

  @Autowired
  @Qualifier("securityContextExecutor")
  private val executor: Executor? = null

  @DgsData(parentType = DgsConstants.USER.TYPE_NAME)
  fun name(dfe: DgsDataFetchingEnvironment): CompletableFuture<String?> {
      return CompletableFuture.supplyAsync({
            // Logic to get the name at this point the SecurityContext is no longer available.
       }, executor)
  }
}

The security context is not available when we are loading users through a data loader, you could imagine we have a query that is trying to load multiple users and decorate the name of all those users.

The issue arises when graphql-java uses the Asynchronous execution that uses the standard Java ForkJoinPool. So by the time it gets to our fun name the context has been lost because it is running inside a "thread":"ForkJoinPool.commonPool-worker-3" (what we see in our logs) but the main thread (with the Security Context) is running from the Spring request serving thread.

I have been looking into how spring-graphql solves this and they seem to be passing the security context to graphql-java using the Micrometer context propagation library using a SecurityContextThreadLocalAccessor but I can't figure out how the library exactly works.

[luismolina13]

We figured it out 🤦🏽 we were wiring the executor in the wrong place. Will add more in a bit but just so you don't keep looking into it. Thank you for your help!

[srinivasankavitha]

Thanks for the update! That's great. Would you mind updating the above example with the solution that worked for you? We can add it as well to our docs site that way for other folks to refer to.

[luismolina13]

So the issue wasn't at the field level but at the data loader.

@DgsComponent
public class UserDataFetcher {

  @Autowired
  @Qualifier("securityContextExecutor")
  private val executor: Executor? = null

  @DgsData(parentType = DgsConstants.USER.TYPE_NAME)
  fun name(dfe: DgsDataFetchingEnvironment): CompletableFuture<String?> {
    val user: User = dfe.getSource()
    val dataLoader: DataLoader<Long, String?> = dfe.getDataLoader(UserNameDataLoader::class.java)
    return dataLoader.load(user.id)
  }
}

We didn't need to do anything there, but we needed to pass the executor in the MappedBatchLoader

@DgsDataLoader
class BrandDataLoader @Autowired constructor(
    private val userRepository: UserRepository,
) : MappedBatchLoader<Long, String?> {

    @Autowired
    @Qualifier("securityContextExecutor")
    private val executor: Executor? = null

    override fun load(keys: Set<Long>): CompletionStage<Map<Long, String?>> {
        return CompletableFuture.supplyAsync({
            userRepository.getNamesByIds(keys).associateBy { it.id!! }
        }, executor)
    }
}

I think it could be useful to enhance the example here: https://netflix.github.io/dgs/data-loaders/#using-spring-features-such-as-securitycontextholder-inside-a-completablefuture in the context of a DataLoader, that's what threw me off at the beginning.

Really appreciate you pointing me in that direction!

[srinivasankavitha]

Great, we will update our docs to do that. Thanks so much for the details!