spring-projects / spring-security

Spring Security
http://spring.io/projects/spring-security
Apache License 2.0
8.71k stars 5.85k forks source link

@PreAuthorize should not apply method security to inherited concrete methods #15352

Closed MartinHaeusler closed 1 month ago

MartinHaeusler commented 2 months ago

Describe the bug I don't know the exact spring security patch version where this behavior changed, but in 6.2.4 I had a setup like this:

@RestController
@PreAuthorize("hasAdminRole()") // expression doesn't matter
public class MyRestController {

    @ResponseBody
    @GetMapping("/status")
    public String someEndpoint(){
        return "foo";
    }

   @ExceptionHandler
   public ResponseEntity<String> handleAccessDeniedException(ServletRequets request, AccessDeniedException e){
        // .... build some nice string... 
   }

}

In Spring Security 6.2.4, if an request comes in that does not match the @PreAuthorize condition, then Spring Security creates an AccessDeniedException, and hands it over to the handleAccessDeniedException(...) method. Everything was fine.

In Spring Security 6.3.1 (and maybe earlier as well, not sure, this is the version I'm currently upgrading to), something else happens:

Overall, this results in a HTTP 403 for the client, no matter what the handleAccessDeniedException method would have done, because it never gets called.

The workaround is to move all @ExceptionHandler methods to an external @ControllerAdvice class, as those are not subject to the method security imposed by the class-level @PreAuthorize annotation. In Spring Security 6.2.4 this worked out of the box.

I think this is a weird breaking change and a potential pitfall for developers. Nobody would think that spring would intercept itself, applying method security on an exception handler method.

marcusdacoregio commented 2 months ago

Hi @MartinHaeusler, thanks for the report.

Are you sure that it used to work in Spring Security 6.2.x? I'm trying a sample with Spring Boot 3.2.4, therefore Spring Security 6.2.4 and I see the handler method not being invoked:

Failure in @ExceptionHandler com.example.springsecurityexamples.MyRestController#handleAccessDeniedException(AccessDeniedException)

Are you able to provide a minimal, reproducible sample where I can just change versions to simulate the problem?

marcusdacoregio commented 1 month ago

I just checked against all the supported branches and that doesn't work in any of them.

MartinHaeusler commented 1 month ago

@marcusdacoregio I apologize for the delay, I've been on vacation for the last two weeks. There is one detail we had in our codebase which I didn't include in the example (because I thought it doesn't matter):

We had our @ExceptionHandler methods defined in an abstract class AbstractRestController from which all our REST controllers derived. This used to be our way to share the handler logic. Our AbstractRestController had no spring security annotations at all. With the upgrade from 6.2.4 to 6.3.1, those handlers stopped working for the reasons listed in the opening post. This may or may not have been an intentional change which is why I thought I should report it.

Also, the popular tutorial site Baeldung recommends the abstract base class approach.

We've moved all our @ExceptionHandlers into a @ControllerAdvice class and eliminated our AbstractRestController, it works fine that way.

In general, I would question if it is a good idea to apply any method security rules to a method that is annotated as @ExceptionHandler. I could imagine that this trips up developers and the resulting exceptions are quite hard to understand.

kse-music commented 1 month ago

I just checked against all the supported branches and that doesn't work in any of them.

This bug maybe exist in all branches.

When an exception occurs, the ExceptionHandlerdefined in Controller is used first, so when the handleAccessDeniedExceptionmethod is executed, it is also intercepted by AuthorizationManagerBeforeMethodInterceptor

Here is the test code

@RequestMapping("test")
@RestController
@PreAuthorize("hasRole('admin')")
@EnableMethodSecurity
@SpringBootApplication
public class TestApplication {

    public static void main(String[] args) {
        SpringApplication.run(TestApplication.class, args);
    }

    @Bean
    static SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception {
        return http
                .authorizeHttpRequests(c -> c.anyRequest().permitAll())
                .build();
    }

    @GetMapping("list")
    public String list(String query) {
        return query;
    }

    @ExceptionHandler
    public ResponseEntity<String> handleAccessDeniedException(AccessDeniedException e){
        return ResponseEntity.ok(e.getMessage());
    }

}