ch4mpy / spring-addons

Ease spring OAuth2 resource-servers configuration and testing
Apache License 2.0
530 stars 87 forks source link

#100 JwtIssuerReactiveAuthenticationManagerResolver bean from autoconfiguration bugfix #101

Closed lArtiquel closed 1 year ago

lArtiquel commented 1 year ago

Description Hello 👋

I spotted a small bug while testing a multi-tentancy feature.

Expected Result In my use-case scenario, REST API endpoint call to the resource server with auth token issued by Realm not specified in the com.c4-soft.springaddons.security.issuers[*].location list, should return 401 (Unauthorized) Status Code instead of 500 SC.

Debugging process explained

java.lang.NullPointerException: null at org.springframework.security.oauth2.server.resource.authentication.JwtIssuerReactiveAuthenticationManagerResolver$ResolvingAuthenticationManager.lambda$authenticate$1(JwtIssuerReactiveAuthenticationManagerResolver.java:145) ~[spring-security-oauth2-resource-server-5.5.4.jar:5.5.4] Suppressed: reactor.core.publisher.FluxOnAssembly$OnAssemblyException: Error has been observed at the following site(s): __checkpoint ⇢ org.springframework.security.web.server.authentication.AuthenticationWebFilter [DefaultWebFilterChain] checkpoint ⇢ org.springframework.security.web.server.context.ReactorContextWebFilter [DefaultWebFilterChain] *checkpoint ⇢ org.springframework.security.web.server.header.HttpHeaderWriterWebFilter [DefaultWebFilterChain] __checkpoint ⇢ org.springframework.security.config.web.server.ServerHttpSecurity$ServerWebExchangeReactorContextWebFilter [DefaultWebFilterChain] checkpoint ⇢ org.springframework.security.web.server.WebFilterChainProxy [DefaultWebFilterChain] *checkpoint ⇢ org.springframework.boot.actuate.metrics.web.reactive.server.MetricsWebFilter [DefaultWebFilterChain] *__checkpoint ⇢ HTTP GET "/dummy" [ExceptionHandlingWebHandler]


- After checking
`org.springframework.security.oauth2.server.resource.authentication.JwtIssuerReactiveAuthenticationManagerResolver$ResolvingAuthenticationManager.ResolvingAuthenticationManager#authenticate()`, I found out that the issue is on the following line when we try to resolve `AuthenticationManager` against non-existent manager in the map.

return this.issuerAuthenticationManagerResolver.resolve(issuer).switchIfEmpty(Mono.error(() -> { return new InvalidBearerTokenException("Invalid issuer " + issuer); }));


- Then after realizing that it's a Spring Security dependency, I tracked down where that `issuerAuthenticationManagerResolver` bean gets created that is used here. I found it in the `AddonsWebSecurityBeans.java` config class, here it is:
@ConditionalOnMissingBean
@Bean
ReactiveAuthenticationManagerResolver<ServerWebExchange> authenticationManagerResolver(
        OAuth2ResourceServerProperties auth2ResourceServerProperties,
        SpringAddonsSecurityProperties addonsProperties,
        Converter<Jwt, Mono<AbstractAuthenticationToken>> jwtAuthenticationConverter) {
    final var jwtProps = Optional.ofNullable(auth2ResourceServerProperties)
            .map(OAuth2ResourceServerProperties::getJwt);
    // @formatter:off
    Optional.ofNullable(jwtProps.map(OAuth2ResourceServerProperties.Jwt::getIssuerUri)).orElse(jwtProps.map(OAuth2ResourceServerProperties.Jwt::getJwkSetUri))
        .filter(StringUtils::hasLength)
        .ifPresent(jwtConf -> {
            log.warn("spring.security.oauth2.resourceserver configuration will be ignored in favor of com.c4-soft.springaddons.security");
        });
    // @formatter:on

    final Map<String, Mono<ReactiveAuthenticationManager>> jwtManagers = Stream.of(addonsProperties.getIssuers())
            .collect(Collectors.toMap(issuer -> issuer.getLocation().toString(), issuer -> {
                ReactiveJwtDecoder decoder = issuer.getJwkSetUri() != null
                        && StringUtils.hasLength(issuer.getJwkSetUri().toString())
                                ? NimbusReactiveJwtDecoder.withJwkSetUri(issuer.getJwkSetUri().toString()).build()
                                : ReactiveJwtDecoders.fromIssuerLocation(issuer.getLocation().toString());
                var provider = new JwtReactiveAuthenticationManager(decoder);
                provider.setJwtAuthenticationConverter(jwtAuthenticationConverter);
                return Mono.just(provider);
            }));

    log.debug(
            "Building default JwtIssuerReactiveAuthenticationManagerResolver with: {} {}",
            auth2ResourceServerProperties.getJwt(),
            Stream.of(addonsProperties.getIssuers()).toList());
    return new JwtIssuerReactiveAuthenticationManagerResolver(
            (ReactiveAuthenticationManagerResolver<String>) jwtManagers::get);
}

- As we can see, `Resolver` does return a null in case if Manager with such issuer URI does not exist in the map
    return new JwtIssuerReactiveAuthenticationManagerResolver(
            (ReactiveAuthenticationManagerResolver<String>) jwtManagers::get);
Therefore, I just patched that line to:
    return new JwtIssuerReactiveAuthenticationManagerResolver(managerName ->
            jwtManagers.getOrDefault(managerName, Mono.empty()));

And used that Bean instead and after that I got expected 401 SC in the response 🎉 .

**Additional info**
Reactive (WebFlux) application.
Spring Addon dependency used:
    `implementation "com.c4-soft.springaddons:spring-addons-webflux-jwt-resource-server:$5.4.0"`

I could not use 6.x versions b/c of my Spring Boot (2.5.9) version. But I see in the source code that the bug is still there ;)

**Questions**
In the case of approval, could you please release that fix for 5.x version (e.g. 5.4.1) for me so that I could use it without _hacking_ this autocofiguration bean?

Thanks 🙏 
lArtiquel commented 1 year ago

Resolves #100 issue Sorry for duplicating, didn't plan to create a PR at the beginning

ch4mpy commented 1 year ago

There is a 5.x branch and you should be able to create another pull-request, and sure, your patch should be applied there too.

I suggest you git checkout 5.x; git cherry-pick c828d2dd1b806536adb87cf9fa102722ca7d5db1 then create a second pull-request targeting ch4mpy:5.x.