Skip to content

[6.2] getLastAuthenticationError(): Return value must be of type ?AuthenticationException, string returned #49166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
AdamReece-WebBox opened this issue Jan 31, 2023 · 4 comments

Comments

@AdamReece-WebBox
Copy link

AdamReece-WebBox commented Jan 31, 2023

Symfony version(s) affected

6.2.5

Description

In an authenticator handler, e.g. \App\Security\AppAuthenticator::authenticate(), throwing an exception does not set the session correctly. This is based on the guide How to Write a Custom Authenticator.

For example if you throw a bad credentials exception because a user identifier is not found:

/** @var User|null $user Local user entity. */
if (!($user = $this->userRepository->loadUserByIdentifier($username))) {
    throw new BadCredentialsException();
}

Then in your authenticate controller you grab that error from the AuthenticationUtils service:

$error = $authenticationUtils->getLastAuthenticationError();

This will cause an internal error because \Symfony\Component\Security\Http\Authentication\AuthenticationUtils::getLastAuthenticationError() has a return type hint of ?AuthenticationException, however when the thrown exception is read from the session here:

$authenticationException = $session->get(SecurityRequestAttributes::AUTHENTICATION_ERROR);

The content of $authenticationException will be string instead of the original exception instance thrown, thus invalid for the return type hint. This results with a fatal error:

Symfony\Component\Security\Http\Authentication\AuthenticationUtils::getLastAuthenticationError(): Return value must be of type ?Symfony\Component\Security\Core\Exception\AuthenticationException, string returned

Logging in successfully won't produce a problem because the $error will be null.

How to reproduce

  1. Build a custom authenticator as per this guide, even if it's a classic Doctrine entity username/password based authenticator.
  2. Fetch the last authentication error in your authentication controller.
  3. Submit some bad credentials to your login form so that an exception is thrown.

Possible Solution

The session would need to contain a serialised instance of the authentication exception thrown, or the getLastAuthenticationError() would need to permit the return of a string as part of a union type with AuthenticationException and null.

Additional Context

I don't recall getting this problem in Symfony 6.1 or earlier though I could be wrong, we've only noticed it since 6.2.

@xabbuh
Copy link
Member

xabbuh commented Jan 31, 2023

Can you create a small example application that allows to reproduce your issue?

AdamReece-WebBox added a commit to AdamReece-WebBox/symfony-issue-49166 that referenced this issue Jan 31, 2023
Visit URI "/account/login" then try any credentials, because they'll all be wrong.
(Unless of course you bother to make a valid user in the Doctrine DB.)
@AdamReece-WebBox
Copy link
Author

Can you create a small example application that allows to reproduce your issue?

heh really? I wasn't expecting you'd need that much from me, but here you go:

https://github.com/AdamReece-WebBox/symfony-issue-49166

Visit URI "/account/login", try any credentials (because they'll all be wrong), then the invalid return type error will happen.

@MatTheCat
Copy link
Contributor

MatTheCat commented Feb 1, 2023

Thanks for the reproducer!

It appears you set the exception message in the session, whereas you should store the exception itself: https://github.com/AdamReece-WebBox/symfony-issue-49166/blob/cf3731a3dcf00bc1c70bb6623fa013c0b8021e96/src/Security/AppAuthenticator.php#L284

Also,

If your custom authenticator is a login form, you can extend from the AbstractLoginFormAuthenticator class instead to make your job easier.

https://symfony.com/doc/current/security/custom_authenticator.html

@AdamReece-WebBox
Copy link
Author

Hi @MatTheCat,

Thank you for the insight within the onAuthenticationFailure() function. We must have got that approach from a dated guide that's been valid until recently on version 6.2. Can confirm passing the exception instance resolves this issue when logging in with bad credentials.

I've also looked into AbstractLoginFormAuthenticator -- it doesn't look like our authenticator changes by much, but adds a bit of compliance standards to what we had, so we'll take it. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants