SpringBoot Bean validation is executed before @PreAuthorize

@RestController
public class SomeController {

    @PreAuthorize("@security.isAuthenticated()")
    @PostMapping("/todo")
    public void add(@Valid @RequestBody dto: SomeDto) {
    }
}

Method isAuthenticated should be executed before deserialization and validation of action parameters.

Comment From: wilkinsona

This is working as designed (and is also out of Boot's control). Spring Security enforces method level security as part of the method being invoked. On the other hand, when an argument of a method on a controller is annotated with @Valid, Spring MVC performs validation prior to invoking the method. This ensures that anything that receives the argument when the method is invoked can be certain that the argument is valid.

Comment From: weburnit

@raderio I've just found solution last weekend and it still makes more sense rather than a trick

    @InitBinder("groupAssign")
    @PreAuthorize("hasPermission('ACL', 'WRITE')")
    protected void initBinder(WebDataBinder binder) {
        AssignmentValidation assignmentValidation = new AssignmentValidation(this.groupRepository,
            this.repo);
        binder.addValidators(assignmentValidation);
    }

When it comes to Validation, I believe that you still have your own validation through an @InitBinder, right there you can add your @PreAuthorize for that particular case which still cover your context: I need to PreAuthorize for groupAssign before WRITING...

hope that helps

Comment From: lukasbradley

@weburnit Do you have a more complete example you could share with us? I don't understand where you are placing the binder. Is it in the RestController itself? At the Configuration level? Any help appreciated.

Comment From: ghost

I think everybody agree with the simply rule "Security First" . Sorry but for me this is a big issue why do we want to run the validation and return validation error if somebody has not event permission to this endpoint? Also the explanation for me its not professional, its sound like "we don't care with the basic security rules the design is more important than the security".

Now how we can be confident by using spring security once we know that we can run some other methods before we actually authorise user. The simple question after all is: what else other than validation is running before security check ?

Comment From: wilkinsona

@lukasw44 Rather than apologising for being rude in what you've written earlier in the same comment, please consider rewriting it so that it's no longer rude and please observe the code of conduct in future.

As has already been explained above, the behaviour that you dislike is out of Spring Boot's control. If you would like to pursue this, and are prepared to do so in a polite and productive manner, please raise it with the Spring Security team. You may want to chat with them on Gitter in the first instance.

Comment From: ghost

@wilkinsona sorry for that I just want to express that I really dislike this behaviour. I updated my comment so now I hope is fine. I will definitely raise this ticket with spring security team and chat with them on Gitter.

Comment From: lukasbradley

He wasn't being that rude, Andy.

On Fri, Dec 14, 2018, 6:58 AM Andy Wilkinson <notifications@github.com wrote:

@lukasw44 https://github.com/lukasw44 Rather than apologising for being rude in what you've written earlier in the same comment, please consider rewriting it so that it's no longer rude and please observe the code of conduct https://github.com/spring-projects/spring-boot/blob/master/CODE_OF_CONDUCT.adoc in future.

As has already been explained above, the behaviour that you dislike is out of Spring Boot's control. If you would like to pursue this, and are prepared to do so in a polite and productive manner, please raise it with the Spring Security team. You may want to chat with them on Gitter https://gitter.im/spring-projects/spring-security in the first instance.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/spring-projects/spring-boot/issues/10157#issuecomment-447303936, or mute the thread https://github.com/notifications/unsubscribe-auth/AAgHzlqZEoSu2sCfsC6h6pwT4w_DAwbRks5u45JlgaJpZM4PL7-1 .

Comment From: wilkinsona

@lukasbradley The degree of rudeness doesn't really matter. The comment, prior to being edited, self-described as rude. It also made derogatory comments, which are prohibited by the CoC, about an attempt to help and explain that had been made earlier in the issue.

Building a welcoming and friendly community is something that we take very seriously. We want people who are considering getting involved to feel confident that if they do so they won't regret it. Part of that is enforcing the CoC and encouraging people to be polite and considerate in their communication.

Comment From: j0rdanit0

Because of this behavior, I've decided to move all validation annotations out of my @Controllers and into my @Services. This works for me because I already follow the design pattern of delegating all business logic to the service layer.

@RestController
public class LibraryController {
    private final LibraryService libraryService;

    @PreAuthorize( "hasRole( 'LIBRARIAN' )" ) //@PreAuthorize executes first
    @PostMapping( "/book" )
    public Book postBook( @RequestBody BookRequest bookRequest ) {
        return libraryService.addBook( bookRequest );
    }
}

@Service
@Validated
public class LibraryService {

    //@Valid executes after @PreAuthorize passes in the controller
    public Book addBook( @Valid BookRequest bookRequest ) {
        // do something
    }
}

This way, security is applied via @PreAuthorize first. If that passes, the controller method is more or less just a one-liner that calls the service layer with the same objects it received in the controller layer. Now the service layer performs all validation that I originally put in my controller layer.

I hope this helps.

However, I believe this problem is quite dangerous and should be reconsidered as "working as designed." The fact that many (most) tutorials online describe adding validation annotations to controller endpoints without talking about this issue leads to many developers combining what they've learned about validation with what they've learned about authorization, and producing this behavior without realizing it. I only found it because I wrote automated tests that failed to meet my expectations. Not all developers do this either.

Comment From: wilkinsona

@j0rdanit0 Thanks for sharing your experience. As I said above, the design is out of Spring Boot's control. To have a chance, your suggestion really needs to be seen by the Spring Framework and Spring Security teams (/cc @rstoyanchev @rwinch).

Comment From: rwinch

As far as I know this is outside of the control of the Spring Security team as well. The ordering is determined by how Spring MVC invokes the validation and the controller method, so something would need to be done within that code.

From a pragmatic standpoint, I'm not sure if Security can consistently be applied before any validation / conversion. Spring Security allows using the method arguments in the SpEL expressions and so the method arguments must be able to be resolved. That means a certain amount of validation is required before the SpEL expression is invoked. For example, if there is a parameter of type int expected on the controller and a user passes in X that value cannot be passed. If the SpEL expression attempts to use the int argument and it cannot be converted, then the Security expression cannot be evaluated.

As to the question :

what else other than validation is running before security check

If authorization fails, Spring Security will prevent that method from being invoked and nothing else.

If you want authorization to happen earlier, you can setup authorization rules based upon the HTTP Requests. Note that by definition, more work will need to be done before authorization occurs if you are annotating your controllers with Security Annotations. This is due to the fact that the logic to determine which controller method is invoked must be performed. In addition, the logic for determining the arguments will also need to be performed to ensure that the method arguments can be resolved.

Comment From: j0rdanit0

@rwinch Thank you for that detailed response. Indeed, as I've thought more about this after discovering it, I've realized that there are several ways to see this behavior even without involving jakarta validation, such as your example about an int being passed a non-numeric value. A 400 would be returned instead of 403, which gives a bad actor information about an endpoint that they shouldn't have been allowed to know about in the first place.

Let's draw a parallel with the real world. Let's say I am a thief that wants to enter a secure building. When I drive up to the gate, a robotic voice says "Sorry, your truck is too tall to fit into our garage." So I come back in my car and it says "Sorry, you're not allowed access." That's odd, right? Why did they care if my truck would fit if I'm not even allowed to enter in the first place? But now I know a bit more about their infrastructure, and how to potentially gain additional information as well.

If we take a step back from the technical reasons for why this behavior exists in Spring, I think the expectation that I (and perhaps others) had in mind was that @PreAuthorize should be executed first. After all, it is called "PreAuthorize" and its documentation says it can be used to decide whether a method invocation is allowed or not. Anything else seems counter-intuitive to me. You are correct that SpEL expressions can reference method arguments, but argument resolution in and of itself isn't required for a lot of basic use cases, such as "hasRole('USER')", which adds to the counter-intuitiveness.

So, if Spring is asked to decide whether a request has access, and is unable to do so, then it must assume that it doesn't, right? The 400 that a non-secured method would return should be a 403 on a secured method. Could that check be made at least?

Comment From: rstoyanchev

@PreAuthorize is pretty well documented as applied through method security via Spring AOP and therefore at the point of method invocation. Spring MVC on the other hand has to validate inputs before invoking the controller method. As such the two mechanisms are independent of each other, and the result is very much as expected for what each mechanism is designed to do.

I do understand the request to change the order, but whether validation or security should take place first can be argued both ways. If you really want to ensure a specific order, apply security earlier at the level of URL security, or apply validation later at the service layer as you have done. These are both good practices IMO. Security works best when applied at multiple levels.

Comment From: jwedel

@rwinch @wilkinsona

Thanks for your explanation. Despite all the "technical details" (please for forgive me, we'll come to that later), I have to agree with @j0rdanit0. This is not what as user of spring security expects.

I did some testing and found out that the flows look like this:

Path-based auth in security config: Authentication -> Authorization -> JSON Parsing -> Validation

With @PreAuthorized: Authentication -> JSON Parsing -> Validation -> Authorization

This is generally a security risk IMO. Doing validation earlier might be acceptable but JSON Parsing before authorization might allow deserialization attacks for unauthorized users.

We, and this might be specific to us, have a custom Spring AOP validation that checks if the given entity is in the data base first. Also this check is done before the authorization is done with PreAuthorized which even allow to DoS the application by an unauthorized user.

Using path-based auth works, however the code is less maintainable and comprehensible as the authorization is completely decoupled from the code (i.e. the controller method).

Regarding the SpEL argument: We actually used SpEL in path based auth as well using the WebExpressionAuthorizationManager to access path variables (e.g. tenant id) which is required for authorization. So that actually worked already before without deserialisation and validation.

So I would really appreciate revisiting the decision that it works as designed and will therefore not be changed.

Thank you.

Comment From: mbdilaver

Even if the @Valid annotation is moved to the service layer, when a user sends a request without a body, an org.springframework.http.converter.HttpMessageNotReadableException is thrown before the @PreAuthorize check.

There's an endpoint that is "protected" by Spring Security, but as long as you send a request that your controller method doesn't like, you can bypass the security annotation. Today it's this error, but tomorrow, who knows which error might be thrown? I support the idea that the security check should be done first before anything else.

Also, giving advice to people, who encounter the problem to change their code flow, using sentences starting with "IMO" is not helpful.

Comment From: abdellahi-brahim

Please consider this issue!