The error message on the default log in pages should always be a generic message so that it does not have any information leakage when AuthenticationException.message includes details about the failure. To help developers, we should also ensure that the failure is logged at the debug level (likely in the AuthenticationManager so that it happens for all failures).

Comment From: Tejas-Teju

Hi @rwinch Can you assign this issue to me? I will work on this

Comment From: jzheaux

Thanks, @Tejas-Teju! Looking forward to another PR from you.

Comment From: Tejas-Teju

@jzheaux

Need your inputs

  1. Can DefaultLoginPageGeneratingFilter.getLoginErrorMessage() be updated for generic messages in login pages? This method returns a Local message for AuthenticationException types but "Invalid credentials" for others. If we can localize the generic message say Bad credentials will that be useful?

  2. The DEBUG messages can be logged at ProviderManager?

We can even log the error at OneTimeTokenAuthenticationProvider and re-throw the exception as BadCredentialsException (Refer PR)

Comment From: jzheaux

The change is to not check for an exception message. Because of that, the answer is always "Invalid Credentials". That means this:

String errorMsg = loginError ? getLoginErrorMessage(request) : "Invalid credentials";

should be able to change to:

String errorMsg = "Invalid Credentials";

and getLoginErrorMessage should not be necessary anymore.

The format for failure logs that we usually follow is:

{what the code failed to do} since {reason for the failure}

For example,

Failed to authenticate token since it was either expired or missing

As for Provider Manager, I'd probably have a generic message like:

Denying authentication since all attempted providers failed

for the end and a more contextual message for the AccountStatusException and InternalAuthenticationServiceException catch block.

Comment From: jzheaux

We can even log the error at OneTimeTokenAuthenticationProvider and re-throw the exception as BadCredentialsException

Good idea, @Tejas-Teju, please go ahead an just focus on ProviderManager and DefaultLoginPageGeneratingFilter, though, for this ticket. And would you mind creating a separate ticket to improve logging in One-Time Token? That way we can review that entire feature holistically.

Comment From: Tejas-Teju

Thanks @jzheaux

There was a test case for getting the error message in KOREA; hence, I thought we'll have to render the message in Local language.

Sure, I'll raise a separate ticket for logging.