spring-projects / spring-data-couchbase

Provides support to increase developer productivity in Java when using Couchbase. Uses familiar Spring concepts such as a template classes for core API usage and lightweight repository style data access.
https://spring.io/projects/spring-data-couchbase
Apache License 2.0
277 stars 191 forks source link

SecurityContextHolder is not propagated into @Transactional methods #1944

Closed leonidr82 closed 1 month ago

leonidr82 commented 5 months ago

We have next state: One of the services (say ResolverService service) calling another service (say SpaceService service). Resolver service invoking method resolverA method with call to some api (say spaceA) from Space service.

@Service
class ResolverService{

SpaceService spaceService;

public boolean resolverA(Object someData){
     spaceService.spaceA(someData);
     ....
    return true;
} 

}

@Service
@Transactional
class SpaceService {

public boolean spaceA(Object someData){
     //Do stuff
    someInsideMethodCalls(someData);
     ....
    return true;
} 

}

At scope of resolverA we have Security context - e.g. SecurityContextHolder.getContext().getAuthentication() holds relevant jwt authentication object. When we going inside of SpaceService#spaceA - Security context has been nullified (e.g. SecurityContextHolder.getContext().getAuthentication() returns null).

while in given case i would expect to get this context.

If @Transactional attribute removed from SpaceService class - Security context presented and passed as it should.

Security context should be passed correctly if @Transacitonal attribute in use.

mikereiche commented 5 months ago

The default strategy for SecurityContextHolder is MODE_THREADLOCAL. The spring-data-implemenation of @Transactional does not execute the method in the same thread as it was called from. Using MODE_GLOBAL will allow it to be retrieved from a different thread, but is not suitable for multi-user, multi-threaded applications.

I would suggested retrieving the authorization in the caller and passing it as an argument to the service.

@Service
class ResolverService{

SpaceService spaceService;

public boolean resolverA(Object someData){
     spaceService.spaceA(someData, SecurityContextHolder.getContext()); // <-- pass arg
     ....
    return true;
} 

}

@Service
@Transactional
class SpaceService {

public boolean spaceA(Object someData, SecurityContext ctx){ // <-- add as arg
     SecurityContextHolder.setContext(ctx);  // <--- set
     //Do stuff
    someInsideMethodCalls(someData);
     ....
    return true;
} 

}
leonidr82 commented 5 months ago

hi @mikereiche thanks for the response. In any case - we cannot use GLOBAL in our case and we're not using LOCAL - we're using INHERITABLE (specifically for such cases). We did have this solution already, but it's seems to be not perfect - if tomorrow we'll need another context data(say, not security) passed by any other context to be passed - we'll need to add another parameter. In Additional, it means that each invocation for SpaceService service will need to pass such security context parameter - which making code much more complex and will be harder to support.

The spring-data-implemenation of @transactional does not execute the method in the same thread as it was called from.

This is clear and this is one of the reasons why we're using MODE_INHERITABLETHREADLOCAL. Is there any reason not to pass security context if we're using @Transactional? Or, at least to check strategy on the context - and decide if it should be removed and passed over (I don't have issue to remove it - if it's LOCAL, but MODE_INHERITABLETHREADLOCAL seems to be done for such cases)?

mikereiche commented 5 months ago

We did have this solution already, but it's seems to be not perfect - if tomorrow we'll need another context data(say, not security) passed by any other context to be passed - we'll need to add another parameter.

Applications typically have a single uber Context object that holds multiple objects and is passed as an argument to methods that need the context. As additional objects are needed, they are just added to the Context object, there is no need to add arguments. Passing as an argument also avoids any future problems where SomeFutureContextHolder will not work.

Is there any reason not to pass security context if we're using @Transactional?

There is no reason not to pass it. It's just that thread-local mechanisms of SecureContextHolder don't work. SecureContextHolder does have means for you to provide a custom strategy.

Because @Transactional uses reactor, it executes the method in a thread from .environment().transactionsSchedulers().schedulerBlocking(), it will not be in the same thread or a child-thread of the caller. There is no means for having transactionsSchedulers() to provide a custom implementation (that could perhaps provide a Scheduler with a thread which is a child of the current thread). The risk here would be that (a) the creator of the Cluster Environment would forget to provide a transactionSchedulers() to override the default; or (b) the transactionSchedulers() that they did provide did not give a schedulerBlocking() that was a child of the thread that set the SecureContextHandler(); or (c) the schedulerBlocking() would leak schedulers.

The mechanism for passing contextual data through a reactive chain is by writing it to the reactive context using contextWrite(). However - the call to a @Transactional method is simply a method call - nothing from implementation is exposed, so there is no place to do that contextWrite() from the caller. Furthermore, the Couchbase Transaction call ReactiveTransaction.runBlocking(...) does not expose the context either. Even if a ReactiveTransaction.runBlockingWriteContext(...., ctx) was added, the caller would need to know what to what 'ctx' was. It could possibly be something analogous to SecurityContextHolder - such as TransactionContextHolder that uses ThreadLocal. ThreadLocal would work for this since it would be stored and retrieved in the same thread - before the execution of the reactor chain.

leonidr82 commented 5 months ago

There is no reason not to pass it. It's just that thread-local mechanisms of SecureContextHolder don't work. SecureContextHolder does have means for you to provide a custom strategy.

we're using org.springframework.security.core.context.SecurityContextHolder#MODE_INHERITABLETHREADLOCAL . In such case - even if thread will not be the same thread as before - context should be passed to newly created thread from current one, isn't it?

see https://docs.spring.io/spring-security/site/docs/3.0.x/reference/technical-overview.html.

So, the question - why it's not working for MODE_INHERITABLETHREADLOCAL and will it be fixed?

mikereiche commented 5 months ago

In such case ... context should be passed to newly created thread from current one, isn't it?

That is correct, but it's not the case here. Please refer to my previous post where I provided all the details and all the options.

So, the question - why it's not working for MODE_INHERITABLETHREADLOCAL and will it be fixed?

You can file a ticket with spring-boot security for SecurityContextHolder, and they will tell you the same thing.

Burtsev-Alexey commented 1 month ago

I have stumbled on the same problem: when using @Transactional Authentication becomes null (on SecurityContextHolder). I noticed that transactional service methods run on separate TX thread from web request thread where authentication was set, I tried setting SecutityContext strategy to MODE_INHERITABLETHREADLOCAL but surpassingly it didn't help. I spent a few hours and realized that TX thread was created and STARTED!!! before web request thread (could be some thread pool made at application start), and was woken up, so it's not a child thread of web request thread where I have authentication.

It's pretty sad and a major problem for me. I don't even know what to do now, I have used MongoDB (with Spring Data Mongo), and it was working with the same code perfectly. I hoped to try Couchbase....

Please refer to my previous post where I provided all the details and all the options. @mikereiche Your explanation is too smart for me, I haven't understood much. Is it a problem with Couchbase java SDK or with Spring-data-Couchbase?

It's so sad in its current state Couchbase with spring-data plus transactions looks unusable.

mikereiche commented 1 month ago

Is it a problem with Couchbase java SDK or with Spring-data-Couchbase?

Neither. The problem is that SecurityContextHolder does not provide a mode to propagate through a Reactor context. As indicated above, it can be passed as an argument to the service method. Another option is that you provide a custom strategy for SecureContextHolder.

mikereiche commented 1 month ago

investigating what can be done.

mikereiche commented 1 month ago

@Burtsev-Alexey @leonidr82 - there is support in 5.4.0-SNAPSHOT and 5.3.5-SNAPSHOT

There is info here on how to use the snapshots - https://docs.spring.io/spring-data/couchbase/reference/couchbase/configuration.html#snapshot-configuration