AuthenticationFailureBadCredentialsEvent published twice

[mptardy]

Summary

AuthenticationFailureBadCredentialsEvent gets published twice with due the fix of #6009, WebSecurityConfigurerAdapter.java:203.

Actual Behavior

If you create a ApplicationListener<AuthenticationFailureBadCredentialsEvent> and listen to AuthenticationFailureBadCredentialsEvent, you get notified twice when the users provides wrong credentials.

Expected Behavior

Same as AuthenticationSuccessEvent, the AuthenticationFailureBadCredentialsEvent should get published only once.

Configuration

Can be reproduced if you use spring-boot-samples/spring-boot-sample-web-secure-custom and add an ApplicationListener<AuthenticationFailureBadCredentialsEvent>.

Version

Spring Security 5.1.2.RELEASE

Sample

Take spring-boot-samples/spring-boot-sample-web-secure-custom and add an ApplicationListener<AuthenticationFailureBadCredentialsEvent>.

@Component
protected static class LoginAttemptAuthenticationFailureEventListener implements ApplicationListener<AuthenticationFailureBadCredentialsEvent> {
	@Override
	public void onApplicationEvent(AuthenticationFailureBadCredentialsEvent event) {
		System.out.println(event.toString());
	}
}

spring-boot-sample-web-secure-custom.zip

[clevertension]

we should discussion how to publish the failure event between Parent and Child ProviderManager authentication

Parent throw AuthenticationFailureCredentialsExpiredEvent, Child throw AuthenticationFailureBadCredentialsEvent, do we need to publish both the two failure exception? i think we should to do so

so i think it is reasonable to publish twice AuthenticationFailureBadCredentialsEvent here, because they are authenticated from two AuthenticationManager, it is just a coincidence that the two AuthenticationManager throw the same Failure Exception

[clevertension]

in the above scenario, we have 4 provider manger which is linked as parent and child, and it will publish both success event and failure event, but how to define its behavior of event publish?

[clevertension]

@jgrandja i think it is not a bug, can we first discussion it please, see the above picture, the AuthenticatiionSuccessEvent should only publish once, but the failure events should all to be published

[mptardy]

@clevertension my specific case is related AuthenticationFailureBadCredentialsEvent for which I cannot agree with you that it is okey to emit this event multiple times.

@rwinch @jgrandja if this is a bug, would it be possible to fix i in a 5.1.x update?

[clevertension]

@mptardy if we have two different failure event during the invocation of the ProviderManager.authentication(), one is AuthenticationFailureBadCredentialsEvent, another is AuthenticationFailureCredentialsExpiredEvent, should we only publish one, or both two?

[mptardy]

@clevertension in my opinion, each type of failure should get published only once.

But I have a hard time imagine how both these failures could happen during one authentication call. Either your credentials were wrong and you get a AuthenticationFailureBadCredentialsEvent or your credentials were correct but expired and you get a AuthenticationFailureCredentialsExpiredEvent.

[clevertension]

this one authentication call is a recursively call, because it have a field AuthenticationManager parent, ok, your advice is also reasonable, let's wait for the feedback

[jgrandja]

@clevertension Regarding your scenario

Parent throw AuthenticationFailureCredentialsExpiredEvent, Child throw AuthenticationFailureBadCredentialsEvent, do we need to publish both the two failure exception? i think we should to do so

This use case is not valid. If the credentials are bad (wrong password) than both parent and child would throw BadCredentialsException and the same event would get published twice - which is not what we want, hence this is a bug.

Furthermore, if the parent and child AuthenticationManager's each have an AuthenticationProvider that both supports(Class<?> authentication) the same type of Authentication (eg. UsernamePasswordAuthenticationToken) than there may be a case where 2 different types of AuthenticationException may be thrown resulting in 2 different AbstractAuthenticationFailureEvent being published. Although this scenario can happen, depending on the AuthenticationManager hierarchy configuration and the specific Authentication request coming in, this really is an edge case.

I really would like to understand your specific use case more to be sure we are not missing anything. Are you able to provide a sample of your specific use case? I might be able to suggest a different configuration to avoid these kind of edge cases.

[jgrandja]

@mptardy

if this is a bug, would it be possible to fix i in a 5.1.x update?

Yes, it will be back patched to 5.1.x and 5.0.x

[martinwithaar]

I changed my springBootVersion from '2.1.1.RELEASE' to '2.1.2.RELEASE' and can confirm it solved my issue.