Spring Security Resource server configured with JwtIssuerAuthenticationManagerResolver should publish authentication events

Expected Behavior

A single-tenant resource server with default configuration uses ProviderManager for authentication, which publishes authentication success and failure events using a DefaultAuthenticationEventPublisher.

A multi-tenant resource server configured to use JwtIssuerAuthenticationManagerResolver should publish the same authentication events.

Current Behavior

No authentication events are published.

Context

After it is determined that the issuer of the JWT can be trusted (e.g. via TrustedIssuerJwtAuthenticationManagerResolver) and the JWT is deemed valid, I'd like to deduce the tenant and store the tenant ID in a ThreadLocal context.

I could accomplish this using a filter placed after the Spring Security filter chain, but I think a cleaner solution would be listening for AuthenticationSuccessEvent, getting the issuer from the JwtAuthenticationToken, and then populating the tenant context.

Comment From: cselagea

I think this could be accomplished by adding an AuthenticationEventPublisher field initialized to a NullEventPublisher in JwtIssuerAuthenticationManagerResolver and then providing a setter for the publisher field.

private AuthenticationEventPublisher eventPublisher = new NullEventPublisher();

public void setAuthenticationEventPublisher(AuthenticationEventPublisher eventPublisher) {
    Assert.notNull(eventPublisher, "AuthenticationEventPublisher cannot be null");
    this.eventPublisher = eventPublisher;
}

Then in TrustedIssuerJwtAuthenticationManagerResolver, instead of returning a JwtAuthenticationProvider directly, which has no event-publishing capabilities, an AuthenticationEventPublishingJwtAuthenticationProvider (or whatever it would be named) would be returned. This class would implement the AuthenticationManager contract by delegating to a JwtAuthenticationProvider instance and then use the publisher to publish AuthenticationSuccessEvent.

Or more simply, perhaps replace the method reference with the following lambda expression.

return authentication -> {
    Authentication authenticationResult = authenticationProvider.authenticate(authentication);
    eventPublisher.publishAuthenticationSuccess(authenticationResult);
    return authenticationResult;
};

As for authentication failure, since BearerTokenAuthenticationFilter catches AuthenticationException, I suppose you wouldn't want to catch AuthenticationException in AuthenticationEventPublishingJwtAuthenticationProvider, so authentication failure events would be published by BearerTokenAuthenticationFilter.

Comment From: jzheaux

Thanks for the suggestion, @cselagea. I agree that it would be better for the resolver to publish authentication events.

I wonder if JwtIssuerAuthenticationManagerResolver should use ProviderManager, simply to reuse the existing code. ResolvingAuthenticationManager could instead implement AuthenticationProvider. Since it is private, it can easily be changed down the road.

Do you need to set a custom publisher? If not, I'd recommend waiting on introducing the setter as I find this to be uncommon.

If we are agreed, I'm happy to make the changes I described.

Comment From: cselagea

@jzheaux No, I don't need to set a custom publisher.

Reusing ProviderManager makes sense. When you say ResolvingAuthenticationManager could instead implement AuthenticationProvider, I assume you mean the constructors would then be modified to look something like this?

this.authenticationManager = new ProviderManager(new ResolvingAuthenticationProvider(...));

Comment From: jzheaux

Yes, @cselagea, that's what I mean.

Comment From: cselagea

Great, I'm in agreement. Thank you, @jzheaux

Comment From: cselagea

@jzheaux could I give it a shot to create a PR for this?

Comment From: jzheaux

Yes, @cselagea, that sounds great. Though I believe I misspoke when I said the setter should be unnecessary at first. Since an AuthenticationEventPublisher cannot be constructed without an ApplicationEventPublisher, the only reasonable default is a null publisher, which will need to be overridden to get events published.

Comment From: cselagea

@jzheaux, understood.

Should I base my work on the 5.7.x branch (to match the milestone) or main (as per the contribution guidelines)?

Comment From: jzheaux

Actually, will you please base it off of 5.8.x? The ticket appears to be out of date.

Comment From: cselagea

@jzheaux, done. I opened PR #11281.

Comment From: jzheaux

@cselagea, I'm curious what you are doing in your application now to get events. This is what I would try in my own application:

@Bean 
JwtIssuerAuthenticationManagerResolver withEvents(AuthenticationEventsPublisher publisher) {
    Map<String, AuthenticationManager> managers = new HashMap<>();
    for (String issuer : issuers) {
        JwtDecoder decoder = new SupplierJwtDecoder(() -> JwtDecoders.fromIssuerLocation(issuer));
        ProviderManager providers = new ProviderManager(new JwtAuthenticationProvider(decoder));
        providers.setAuthenticationEventPublisher(publisher);
        managers.put(issuer, providers);
    }
    return new JwtIssuerAuthenticationManagerResolver(managers::get);
}

Comment From: cselagea

@jzheaux, here's what I've currently got. With the addition of SupplierJwtDecoder in 5.6, I think I can simplify it to look more like your code sample while maintaining lazy initialization of JwtDecoder.

@Bean
public JwtIssuerAuthenticationManagerResolver authenticationManagerResolver(
        IssuerRegistry issuerRegistry,
        AuthenticationProviderFactory authenticationProviderFactory,
        AuthenticationEventPublisher eventPublisher) {

    Predicate<String> trustedIssuerTest = issuerRegistry.getIssuers()::contains;
    AuthenticationManagerResolver<String> authenticationManagerResolver =
            new TrustedIssuerJwtAuthenticationManagerResolver(trustedIssuerTest, authenticationProviderFactory, eventPublisher);
    return new JwtIssuerAuthenticationManagerResolver(authenticationManagerResolver);
}

class TrustedIssuerJwtAuthenticationManagerResolver implements AuthenticationManagerResolver<String> {

    private final Map<String, AuthenticationManager> authenticationManagers = new ConcurrentHashMap<>();

    // other fields and constructor omitted

    @Override
    public AuthenticationManager resolve(String issuer) {
        if (this.trustedIssuer.test(issuer)) {
            AuthenticationManager authenticationManager = this.authenticationManagers.computeIfAbsent(issuer,
                    key -> {
                        AuthenticationProvider authenticationProvider = authenticationProviderFactory.createProvider(issuer);

                        return authentication -> {
                            Authentication authenticationResult = authenticationProvider.authenticate(authentication);
                            eventPublisher.publishAuthenticationSuccess(authenticationResult);
                            return authenticationResult;
                        };
                    });
            logger.debug("Resolved AuthenticationManager for issuer '{}'", issuer);
            return authenticationManager;
        } else {
            logger.debug("Did not resolve AuthenticationManager since issuer is not trusted");
        }
        return null;
    }

}

class AuthenticationProviderFactory {

    // fields and constructor omitted

    public AuthenticationProvider createProvider(String issuer) {
        JwtDecoder jwtDecoder = jwtDecoderFactory.createDecoder(issuer);
        JwtAuthenticationProvider authenticationProvider = new JwtAuthenticationProvider(jwtDecoder);
        authenticationProvider.setJwtAuthenticationConverter(jwtAuthenticationConverter);
        return authenticationProvider;
    }

}

class IssuerJwtDecoderFactory implements JwtDecoderFactory<String> {

    // fields and constructor omitted

    @Override
    public JwtDecoder createDecoder(String issuer) {
        NimbusJwtDecoder jwtDecoder = JwtDecoders.fromOidcIssuerLocation(issuer);
        jwtDecoder.setJwtValidator(jwtValidatorFactory.createValidator(issuer));
        return jwtDecoder;
    }

}

Comment From: jzheaux

Thanks, @cselagea, that helps.

It seems like you need a custom AuthenticationManagerResolver anyway in order to set the JwtAuthenticationConverter and JwtValidator, so I don't think that setting an event publisher on JwtIssuerAuthenticationManagerResolver is going to reduce your configuration any. You'd either call the setter there or you'd call it on the ProviderManager you are constructing.

Given that, I'm going to close this issue for now, but please feel free to reopen it if it feels like I misunderstood.

As a side note I wonder if something like this would simplify your arrangement:

@Cacheable(unless="#result==null")
public AuthenticationManager resolve(String issuer) {
    if (this.trustedIssuer.test(issuer)) {
        ProviderManager manager = new ProviderManager(authenticationProviderFactory::createProvider);
        manager.setAuthenticationEventPublisher(publisher);
        return manager;
    }
    return null;
}

As you said, it could be that this is further simplified by using SupplierJwtDecoder.