spring-projects / spring-security

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

hasAuthority and custom Mono<Boolean> method in @PreAuthorize leads to ConverterNotFoundException error #15209

Open bskorka opened 3 months ago

bskorka commented 3 months ago

Describe the bug After upgrading to Spring Boot 3.3.0 and Spring Security 6.3.0 I've tried to migrate my single Mono<Boolean> @PreAuthorize calls to more complex ones as I thought that these tasks make it possible:

However, if we want to use the @PreAuthorize connected with a Mono<Boolean> and hasAuthority we are getting an error No converter found capable of converting from type [reactor.core.publisher.MonoFlatMap<java.lang.Boolean, ?>] to type [java.lang.Boolean]

To Reproduce

Expected behaviour The "complex" @PreAuthorize expression should be handled without isues if we use methods returning Mono<Boolean> and built-in hasAuthority calls.

Sample

@Configuration
@EnableWebFluxSecurity
@EnableReactiveMethodSecurity
public class SecurityConfig {
}
@Slf4j
@RestController
@RequiredArgsConstructor
@RequestMapping(value = "/test")
public class TestController {
    @GetMapping("/values")
    @PreAuthorize("hasAuthority('ppcv:browse') || "
                  + "(hasAuthority('ppcv:browse-scoped') && @userConnectionAuthorizerService.isUserConnected(#id))")
    public Mono<ApiResponse<List<TestObject>>> getAllValues(@RequestParam Integer id,
                                                                                       @RequestParam List<Status> statuses,
                                                                                       @RequestParam(required = false, defaultValue = "false") Boolean latest) {
        return ...
    }
}
public interface UserConnectionAuthorizerService {

    Mono<Boolean> isUserConnected(Long id);

}

When running code in the integration tests I receive:

2024-06-06 18:55:42,135 [parallel-4] ERROR o.s.b.a.w.r.e.AbstractErrorWebExceptionHandler - [5f1ceeb9-3]  500 Server Error for HTTP GET "/test/values?id=1&statuses=APPROVED"
java.lang.IllegalArgumentException: Failed to evaluate expression 'hasAuthority('ppcv:browse') || (hasAuthority('ppcv:browse-scoped') && @userConnectionAuthorizerService.isUserConnected(#id))'
    at org.springframework.security.authorization.method.ReactiveExpressionUtils.lambda$evaluate$0(ReactiveExpressionUtils.java:43)
    Suppressed: reactor.core.publisher.FluxOnAssembly$OnAssemblyException: 
Error has been observed at the following site(s):
    *__checkpoint ⇢ Handler test.TestController#getAllValues(Integer, List, Boolean) [DispatcherHandler]
    *__checkpoint ⇢ test.core.infrastructure.actuator.spring.DefaultActuatorWebFilter [DefaultWebFilterChain]
    *__checkpoint ⇢ test.core.application.spring.http.filter.AdminSecurityFilter [DefaultWebFilterChain]
    *__checkpoint ⇢ AuthorizationWebFilter [DefaultWebFilterChain]
    *__checkpoint ⇢ ExceptionTranslationWebFilter [DefaultWebFilterChain]
    *__checkpoint ⇢ LogoutWebFilter [DefaultWebFilterChain]
    *__checkpoint ⇢ ServerRequestCacheWebFilter [DefaultWebFilterChain]
    *__checkpoint ⇢ SecurityContextServerWebExchangeWebFilter [DefaultWebFilterChain]
    *__checkpoint ⇢ UserIdAuthenticationFilter [DefaultWebFilterChain]
    *__checkpoint ⇢ ReactorContextWebFilter [DefaultWebFilterChain]
    *__checkpoint ⇢ HttpHeaderWriterWebFilter [DefaultWebFilterChain]
    *__checkpoint ⇢ ServerWebExchangeReactorContextWebFilter [DefaultWebFilterChain]
    *__checkpoint ⇢ org.springframework.security.web.server.WebFilterChainProxy [DefaultWebFilterChain]
    *__checkpoint ⇢ HTTP GET "/test/values?id=1&statuses=APPROVED" [ExceptionHandlingWebHandler]
Original Stack Trace:
        at org.springframework.security.authorization.method.ReactiveExpressionUtils.lambda$evaluate$0(ReactiveExpressionUtils.java:43)
        ...
Caused by: org.springframework.expression.spel.SpelEvaluationException: EL1001E: Type conversion problem, cannot convert from reactor.core.publisher.MonoJust<java.lang.Boolean> to java.lang.Boolean
    at org.springframework.expression.spel.support.StandardTypeConverter.convertValue(StandardTypeConverter.java:87)
    at org.springframework.expression.common.ExpressionUtils.convertTypedValue(ExpressionUtils.java:57)
    at org.springframework.expression.spel.ast.SpelNodeImpl.getValue(SpelNodeImpl.java:222)
    at org.springframework.expression.spel.ast.OpAnd.getBooleanValue(OpAnd.java:57)
    at org.springframework.expression.spel.ast.OpAnd.getValueInternal(OpAnd.java:52)
    at org.springframework.expression.spel.ast.SpelNodeImpl.getValue(SpelNodeImpl.java:222)
    at org.springframework.expression.spel.ast.OpOr.getBooleanValue(OpOr.java:56)
    at org.springframework.expression.spel.ast.OpOr.getValueInternal(OpOr.java:51)
    at org.springframework.expression.spel.ast.OpOr.getValueInternal(OpOr.java:37)
    at org.springframework.expression.spel.ast.SpelNodeImpl.getValue(SpelNodeImpl.java:114)
    at org.springframework.expression.spel.standard.SpelExpression.getValue(SpelExpression.java:273)
    at org.springframework.security.authorization.method.ReactiveExpressionUtils.lambda$evaluate$2(ReactiveExpressionUtils.java:39)
    ...
Caused by: org.springframework.core.convert.ConverterNotFoundException: No converter found capable of converting from type [reactor.core.publisher.MonoJust<java.lang.Boolean>] to type [java.lang.Boolean]
    at org.springframework.core.convert.support.GenericConversionService.handleConverterNotFound(GenericConversionService.java:294)
    at org.springframework.core.convert.support.GenericConversionService.convert(GenericConversionService.java:185)
    at org.springframework.expression.spel.support.StandardTypeConverter.convertValue(StandardTypeConverter.java:82)
    ... 260 common frames omitted
jzheaux commented 3 months ago

@bskorka, thanks for the report. Will you please try the following?

Change @PreAuthorize to reference only one bean like so:

@PreAuthorize("@authz.checkEverything(#root)"`)

Then, introduce the above bean in such a way that it checks all three conditions. Can you confirm that this allows you to return Mono<Boolean> as expected?

In the meantime, I will take a look and see what can be done about supporting expression elements that return a mixture of Boolean and Mono<Boolean>. I'm not convinced that this should be added, but it's worth a look. And, we can at least try and improve the error message with additional context.

bskorka commented 3 months ago

Hey @jzheaux! I can confirm that moving all the expressions to one method returning the Mono<Boolean> works fine, as I have used it since support for Mono<Boolean> was introduced in Spring Security 5.8.9(?).

The implementation of this approach looks like this:

public class TestController {
    @GetMapping("/values")
    @PreAuthorize("@userConnectionAuthorizerService.hasUserAccessToConnectionData(#id))")
    public Mono<ApiResponse<List<TestObject>>> getAllValues(@RequestParam Integer id,
                                                            @RequestParam List<Status> statuses,
                                                            @RequestParam(required = false, defaultValue = "false") Boolean latest) {
        return ...
    }
}
public class UserConnectionAuthorizerServiceImpl implements UserConnectionAuthorizerService {
    private final PreAuthService preAuthService;

    @Override
    public Mono<Boolean> hasUserAccessToConnectionData(Long id) {
        return Mono.zip(
                           preAuthService.hasAuthority(Permissions.PPCV_BROWSE.getValue()),
                           preAuthService.hasAuthority(Permissions.PPCV_BROWSE_SCOPED.getValue())
                   )
                   .flatMap(tuple -> tuple.getT1() ? Mono.just(true) :
                           tuple.getT2() ? checkUserConnection(id) : // business check, same as isUserConnected in previous example
                                   Mono.just(false)
                   )
                   .switchIfEmpty(Mono.just(false));
    }
}
public class PreAuthServiceImpl implements PreAuthService {
    public Mono<Boolean> hasAuthority(String authority) {
        return ReactiveSecurityContextHolder.getContext()
                                            .map(SecurityContext::getAuthentication)
                                            .map(authentication -> authentication.getAuthorities().stream()
                                                                                 .anyMatch(grantedAuthority -> grantedAuthority.getAuthority().equals(authority)));
    }
}

I understand that handling the mix of Boolean and Mono<Boolean> might not be easy to implement understandably and satisfyingly. Maybe a reactive variant of the @PreAuthorize annotation or reactiveHasAuthority returning the Mono<Boolean> will work?
I agree that the error message can be improved as I spent some time thinking that the issue is related to my reactive pipe.