spring-projects / spring-security

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

AuthorizeReturnObject should target the authorized object within Spring Data components #15994

Open noshua opened 1 month ago

noshua commented 1 month ago

Describe the bug Using Authorizing Arbitrary Objects of Spring Security in combination with a Pageable Spring Data result fails.

To Reproduce

  1. Add "@PreAuthorize" to an Entity class.
  2. Add "@AuthorizeReturnObject" to a repository class method with return type Page.
  3. Call the repo method.

java.lang.ClassCastException: class org.springframework.security.authorization.method.AuthorizationAdvisorProxyFactory$ContainerTypeVisitor$$Lambda/0x0000791458a2cb00 cannot be cast to class org.springframework.data.domain.Page (org.springframework.security.authorization.method.AuthorizationAdvisorProxyFactory$ContainerTypeVisitor$$Lambda/0x0000791458a2cb00 and org.springframework.data.domain.Page are in unnamed module of loader 'app')

Expected behavior A paged result of security proxied objects should be returned from the repository method.

Sample https://github.com/noshua/authorize-spring-data

jzheaux commented 1 month ago

Thanks for this report, @noshua. Spring Data types are important for @AuthorizeReturnObject to be able to proxy.

For now, you can place @AuthorizeReturnObject on your controller method instead.

Or, if you want to support @AuthorizationReturnObject with Page, you can add this support yourself like so:

private final TargetVisitor page = (proxyFactory, target) -> {
    if (target instanceof PageImpl<?> page) {
        List<Object> content = (List<Object>) proxyFactory.proxy(page.getContent());
        return new PageImpl<>(content, page.getPageable(), page.getTotalElements());
    }
    return null;
};

@Bean
Customizer<AuthorizationAdvisorProxyFactory> addVisitors() {
    return (factory) -> factory.setTargetVisitor(TargetVisitor.of(page, TargetVisitor.defaults()));
}
noshua commented 2 weeks ago

Sorry for the delayed response @jzheaux but I needed some time to investigate.

Putting @AuthorizeReturnObject directly onto @RestController doesn't work at all in combination with ResponseEntity<T> method responses. When I implement a @Component in between JpaRepository and @RestController, with return type T to work around this, but no getter is called in my business logic the AuthorizationDeniedException will happen in JSON response mapping. Then it get's translated into a HttpMessageNotWritableException with status 500 instead of the correct 403 error.

Is it possible to achieve that the @PreAuthorize on T is evaluated when getting the object itself instead of calling it's attributes getters?

I created a new sample branch to show this behaviour: https://github.com/noshua/authorize-spring-data/tree/authorizeController

jzheaux commented 2 weeks ago

Thanks for the sample, that's very helpful.

As with Page any complex object that Spring Security does not yet know how to handle, you can add the appropriate TargetVisitor. A TargetVisitor for ResponseType would be something like:

@Bean
Customizer<AuthorizationAdvisorProxyFactory> addVisitors() {
    return (factory) -> factory.setTargetVisitor(TargetVisitor.of(responseEntity, TargetVisitor.defaults()));
}

private final TargetVisitor responseEntity = (proxyFactory, target) -> {
    if (target instanceof ResponseEntity<?> entity) {
        Object body = entity.getBody();
        HttpHeaders header = entity.getHeaders();
        HttpStatusCode code = entity.getStatusCode();
        return new ResponseEntity<>(proxyFactory.proxy(body), header, code);
    }
    return null;
};

To propagate the AuthorizeDeniedException, I think it would be helpful for Spring Security to add an MVC component that assists with propagation. For now, you can get that to work in the following way:

@ExceptionHandler(HttpMessageNotWritableException.class)
View handleWrite(HttpMessageNotWritableException ex) {
    if (ex.getRootCause() instanceof AuthorizationDeniedException denied) {
       return new AbstractView() {
          @Override
          protected void renderMergedOutputModel(Map<String, Object> model,
                HttpServletRequest request, HttpServletResponse response)
                throws Exception {
             throw ex;
          }
       };
    }
    throw ex;
}

I've added a PR to your sample repo to demonstrate both of these.

jzheaux commented 2 weeks ago

Related to #14717