ProviderManager with multiple AuthenticationProviders erroneously successfully authenticates after a prior provider correctly throws AuthenticationException

[hmolsen]

Some background on what I am doing:
I have a SpringBoot (1.3.3.RELEASE) application with SpringSecurity and I am trying to add two factor authentication. Therefore I extended the DaoAuthenticationProvider with my TfaAuthenticationProvider, which overrides the additionalAuthenticationChecksmethod. This one checks the 2FA token and throws an AuthenticationException if something goes wrong.

protected void additionalAuthenticationChecks(UserDetails userDetails, UsernamePasswordAuthenticationToken authentication) throws AuthenticationException {
    super.additionalAuthenticationChecks(userDetails, authentication);
    if (authentication.getDetails() instanceof TfaWebAuthenticationDetails) {
        User user = (User) userDetails;
        if (user.isTfaEnabled()) {
            String tfaSecret = user.getTfaSecret();
            Integer tfaKey = ((TfaWebAuthenticationDetails) authentication.getDetails()).getTfaKey();
            if (tfaKey != null) {
                try {
                    if (!tfaAuthenticator.verifyCode(tfaSecret, tfaKey, 2)) {
                        System.out.printf("Code %d was not valid", tfaKey);
                        throw new BadCredentialsException("Invalid 2FA code");
                    }
                } catch (InvalidKeyException | NoSuchAlgorithmException e) {
                    throw new InternalAuthenticationServiceException("2FA code verification failed", e);
                }
            } else {
                throw new MissingTfaKeyAuthenticatorException("2FA code is mandatory");
            }
        }
    }
}

Now I add this in my SecurityConfig, for example like that:

@Override
protected void configure(HttpSecurity http) throws Exception {
    // [...]
    http.authenticationProvider(authenticationProvider);
}

    @Bean
    public AuthenticationProvider authenticationProvider() {
        TfaAuthenticationProvider tfaAuthenticationProvider = new TfaAuthenticationProvider();
        tfaAuthenticationProvider.setTfaAuthenticator(new TfaAuthenticator());
        tfaAuthenticationProvider.setUserDetailsService(userDetailsService);
        return tfaAuthenticationProvider;
    }

I was surprised to see me successfully logging in, even when entering a wrong or no 2FA-Code at all. Debugging and code reading gave me insights, which I think might actually be a bug, or something I totally got wrong.

At some point during the authentication process I run into the ProviderManager's method authenticate.
Debugging shows me that my TfaAuthenticationProvideris successfully registered in the List<AuthenticationProvider> providers which now contains:

[0] TfaAuthenticationProvider
[1] AnonymousAuthenticationProvider
[2] DaoAuthenticationProvider

The documentation of the authenticate method says for the case of multiple providers:

If more than one AuthenticationProvider supports the passed Authentication
object, only the first AuthenticationProvider tried will determine the result. No subsequent
AuthenticationProviders will be tried.

Looking at the implementation this is only true for the positive case, i.e., if the first provider (in this case the TfaAuthenticationProvider already gives a result the code runs into

if (result != null) {
    if (eraseCredentialsAfterAuthentication
            && (result instanceof CredentialsContainer)) {
        // Authentication is complete. Remove credentials and other secret data
        // from authentication
        ((CredentialsContainer) result).eraseCredentials();
    }

    eventPublisher.publishAuthenticationSuccess(result);
    return result;
}

but here's what I think is wrong. If the TfaAuthenticationProvider throws an AuthenticationException, for example because no 2FA code is provided, then the ProviderManager catches the Exception and stores it in lastException:

for (AuthenticationProvider provider : getProviders()) {
    if (!provider.supports(toTest)) {
        continue;
    }
    // [...]
    try {
        result = provider.authenticate(authentication);

        if (result != null) {
            copyDetails(authentication, result);
            break;
        }
    }
    // [...]
    catch (AuthenticationException e) {
        lastException = e;
    }
}

after that the for-loop continues and, the second provider (AnonymousAuthenticationProvider) says no support, continue. Then the DaoAuthenticationProvider successfully fills the result. After that the for-loop is exited (break) and the following code continues with both, a populated result and a populated lastException. Unfortunately the result is asked for first, the method then happily returns, skipping the part that would ask for the lastException:

if (result != null) {
    if (eraseCredentialsAfterAuthentication
            && (result instanceof CredentialsContainer)) {
        // Authentication is complete. Remove credentials and other secret data
        // from authentication
        ((CredentialsContainer) result).eraseCredentials();
    }

    eventPublisher.publishAuthenticationSuccess(result);
    return result;
}

// Parent was null, or didn't authenticate (or throw an exception).

if (lastException == null) {
    lastException = new ProviderNotFoundException(messages.getMessage(
            "ProviderManager.providerNotFound",
            new Object[] { toTest.getName() },
            "No AuthenticationProvider found for {0}"));
}

prepareException(lastException, authentication);

throw lastException;

So here's three things. Either
i) I need to manage to replace the DaoAuthenticationProvider instead of adding my extended one
ii) catch (AuthenticationException e) { lastException = e; } should also break;, but the code and variable naming smells like there was a reason to store the "lastException" and to not break, or
iii) if(result != null) should also check for && lastException == null

It feels like from my custom Provider I have just no chance of saying "nope, you shall not pass". Or did I just get something wrong...?

[hmolsen]

My current workaround is hard erasing of the credentials in case of an error, then the later authentication of the DaoAuthenticationProvider fails:

    @Override
    protected void additionalAuthenticationChecks(UserDetails userDetails, UsernamePasswordAuthenticationToken authentication) throws AuthenticationException {
        super.additionalAuthenticationChecks(userDetails, authentication);
        if (authentication.getDetails() instanceof TfaWebAuthenticationDetails) {
            User user = (User) userDetails;
            if (user.isTfaEnabled()) {
                String tfaSecret = user.getTfaSecret();
                Integer tfaKey = ((TfaWebAuthenticationDetails) authentication.getDetails()).getTfaKey();
                if (tfaKey != null) {
                    try {
                        if (!tfaAuthenticator.verifyCode(tfaSecret, tfaKey, 2)) {
                            System.out.printf("Code %d was not valid", tfaKey);
                            authentication.eraseCredentials();
                            throw new BadCredentialsException("Invalid 2FA code");
                        }
                    }
                    catch (InvalidKeyException | NoSuchAlgorithmException e) {
                        authentication.eraseCredentials();
                        throw new InternalAuthenticationServiceException("2FA code verification failed", e);
                    }
                } else {
                    authentication.eraseCredentials();
                    throw new MissingTfaKeyAuthenticatorException("2FA code is mandatory");
                }
            }
        }
    }

[rwinch]

@hmolsen I think you have found a documentation bug. ProviderManager has (for as long as I can remember) behaved this way. The intention is for ProviderManager to stop at the first supported AuthenticationProvider that returns success. If it fails, it continues. This is necessary, for example, for users that keep users in multiple data stores.

One option is to create a composite AuthenticationProvider which performs all the logic for validating the username and password. Then it also validates the token.

[rwinch]

I created #3722 to address the javadoc issue. I'd love to get a pull request with the changes :)

I'm going to close this issue since you have a workaround and I provided an alternative. If you have follow up please feel free to reopen.