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

Security Context has been nullified while using @Transactional attribute on Service method call #1944

Open
leonidr82 opened this issue May 21, 2024 · 5 comments
Labels
status: feedback-provided Feedback has been provided

Comments

@leonidr82
Copy link

leonidr82 commented May 21, 2024

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.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 21, 2024
@mikereiche
Copy link
Collaborator

mikereiche commented May 21, 2024

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;
} 

}

@mikereiche mikereiche added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels May 21, 2024
@leonidr82
Copy link
Author

leonidr82 commented May 22, 2024

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)?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels May 22, 2024
@mikereiche
Copy link
Collaborator

mikereiche commented May 24, 2024

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
Copy link
Author

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
Copy link
Collaborator

mikereiche commented May 29, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: feedback-provided Feedback has been provided
Projects
None yet
Development

No branches or pull requests

3 participants