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

Method security annotation intercepts the ExceptionHandler method #15507

Closed
kse-music opened this issue Aug 1, 2024 · 6 comments
Closed

Method security annotation intercepts the ExceptionHandler method #15507

kse-music opened this issue Aug 1, 2024 · 6 comments
Assignees
Labels
in: core An issue in spring-security-core status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement

Comments

@kse-music
Copy link
Contributor

kse-music commented Aug 1, 2024

Affect all branch

Describe the bug
The exception handler method defined in the Controller is intercepted by the security annotation

To Reproduce
see Sample code

Expected behavior
The exception handler method handleException can be executed

Sample

Demo

@marcusdacoregio
Copy link
Contributor

Hi @kse-music.

Have you verified if the problem persists in the latest version? Looks like it has been fixed via #15352

@marcusdacoregio marcusdacoregio added the status: waiting-for-feedback We need additional information before we can continue label Aug 29, 2024
@marcusdacoregio marcusdacoregio self-assigned this Aug 29, 2024
@marcusdacoregio marcusdacoregio removed the status: waiting-for-triage An issue we've not yet triaged label Aug 29, 2024
@kse-music
Copy link
Contributor Author

@marcusdacoregio The latest version still exists

@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 Aug 29, 2024
@kse-music
Copy link
Contributor Author

Hi @kse-music.

Have you verified if the problem persists in the latest version? Looks like it has been fixed via #15352

In addition, I think this is a common problem, because after the BeanPostProcessor (AbstractAutoProxyCreator )generates the proxy object, subsequent executions of the methods in the object are based on the proxy object, so they will be intercepted. As more, if the object SmartInitializingSingleton interface is implemented, during execution afterSingletonsInstantiated also be intercepted when app is starting .This leads to a direct startup failure.

I'm not sure if I need to add a mechanism to exclude the methods of spring internal interfaces, such as ApplicationListener SmartLifecycle etc.

@jzheaux
Copy link
Contributor

jzheaux commented Aug 29, 2024

While I'm interested in continuing this conversation, I'm not sure how much effort we should put into skipping certain methods when a class-level annotation is in place. Ostensibly, placing @PreAuthorize on a class means "please pre-authorize all the methods declared in this class".

The better solution, I believe, is to instead place the annotation on a parent interface that more clearly indicates which methods or to place the annotation directly on the methods that need authorization.

The technique outlined here may also be of interest.

For my reference, can you please produce a sample that uses a Spring Security version where the above application works as you expect?

@kse-music
Copy link
Contributor Author

I have added sample code

While I'm interested in continuing this conversation, I'm not sure how much effort we should put into skipping certain methods when a class-level annotation is in place. Ostensibly, placing @PreAuthorize on a class means "please pre-authorize all the methods declared in this class".

I agree, but technically it does exist.In my sample code:

  1. curl http://127.0.0.1:8080/user/getName it doesn't execute handleException method
  2. When opening the comment code in PostRestApi, the DemoApplication startup failure

The better solution, I believe, is to instead place the annotation on a parent interface that more clearly indicates which methods or to place the annotation directly on the methods that need authorization.
The technique outlined here may also be of interest.

The fix here solves gh-15352, but also causes gh-13783 to fail, even in 6.4.0-M3. I copied the code in gh-13783 to the gh13789 package of the sample code:
curl http://127.0.0.1:8080/hello it doesn't execute @PreAuthorize method

@marcusdacoregio marcusdacoregio removed their assignment Sep 17, 2024
@jzheaux
Copy link
Contributor

jzheaux commented Sep 18, 2024

I hear your concerns, @kse-music. Since it seems your most recent comments are about how Spring Security searches hierarchically for annotations, let's continue chatting about that at #15352 so that the conversation doesn't become fragmented.

In addition, I think this is a common problem, because after the BeanPostProcessor (AbstractAutoProxyCreator )generates the proxy object, subsequent executions of the methods in the object are based on the proxy object, so they will be intercepted. As more, if the object SmartInitializingSingleton interface is implemented, during execution afterSingletonsInstantiated also be intercepted when app is starting .This leads to a direct startup failure.

Your point here is well-taken though I don't know how we can provide a complete solution to avoid all the methods that folks might want to skip. Better, I think is to either publish a technique or to make the default configuration simple to alter.

For example, if an application is determined to use @PreAuthorize on a concrete class that contains methods it doesn't want that annotation to apply to, applications can craft their own pointcut as follows:

@Bean
@Role(BeanDefinition.ROLE_INFRASTRUCTURE)
static AuthorizationAdvisor preAuthorizeAuthorizationAdvisor() {
    AuthorizationManagerBeforeMethodInterceptor preAuthorize = 
            AuthorizationManagerBeforeMethodInterceptor.preAuthorize();
    Pointcut hasPreAuthorizeAnnotation = preAuthorize.getPointcut();
    String expression = "!@annotation(org.springframework.web.bind.annotation.ExceptionHandler)";
    Pointcut skipExceptionHandler = new AspectJPointcutExpression(expression);
    preAuthorize.setPointcut(Pointcuts.intersection(hasPreAuthorizeAnnotation, skipExceptionHandler));
    return preAuthorize;
}

@jzheaux jzheaux closed this as completed Sep 18, 2024
@jzheaux jzheaux added in: core An issue in spring-security-core type: enhancement A general enhancement status: declined A suggestion or change that we don't feel we should currently apply and removed type: bug A general bug status: feedback-provided Feedback has been provided labels Sep 18, 2024
@jzheaux jzheaux self-assigned this Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core An issue in spring-security-core status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants