Default Log In Page should always be generic message

[rwinch]

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).

[Tejas-Teju]

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

[jzheaux]

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

[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)

[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.

[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.

[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.