[Cookbook][Security] How to Create a Custom Form Password Authenticator

[burci]

In the page https://github.com/symfony/symfony-docs/blob/master/cookbook/security/custom_password_authenticator.rst the next code leads to endless loop:

$passwordValid = $this->encoder->isPasswordValid($user, $token->getCredentials());

because after the validation was successfull credentials being cleard, so the $token->getCredentials() will be null.

It would be better to use same logic then in https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Core/Authentication/Provider/DaoAuthenticationProvider.php#L54 (or the whole DaoAuthenticationProvider)

    public function __construct(EncoderFactoryInterface $encoderFactory, UserCheckerInterface $userChecker)
    {
        $this->encoderFactory = $encoderFactory;
        $this->userChecker = $userChecker;
    }

    public function authenticateToken(TokenInterface $token, UserProviderInterface $userProvider, $providerKey)
    {
        $provider = new DaoAuthenticationProvider($userProvider, $this->userChecker, $providerKey, $this->encoderFactory);
        $authenticatedToken = $provider->authenticate($token);

        $currentHour = date('G');
        if ($currentHour < 14 || $currentHour > 16) {
            throw new AuthenticationException(
                'You can only log in between 2 and 4!',
                100
            );
        }

        return $authenticatedToken;
    }

I can make a pull request if this direction is correct.

(Maybe DaoAuthenticationProvider should renamed to UserProviderBasedAuthenticationProvider or something similar)

[xabbuh]

@burci Can you elaborate a bit more why the current code leads to an infinite loops. Honestly, I don't see the issue yet.

[wouterj]

Hmm, I'm afraid I don't understand the issue either. Can you please explain a bit more?

[burci]

Maybe i was not clear enough, sorry. Let's go step by step:

1, The example at https://github.com/symfony/symfony-docs/blob/master/cookbook/security/custom_password_authenticator.rst was not working for me.

2, The problem was that authenticateToken is being called by all request and the $passwordValid = $this->encoder->isPasswordValid($user, $token->getCredentials()); returned false in the next request, right after the succes authentication.

3, The $this->encoder->isPasswordValid($user, $token->getCredentials()); returned false because $token->getCredentials() returned null.

4, The $token->getCredentials() returned null because after the successful authentication the AuthenticationProviderManager erase the credentials from token: https://github.com/symfony/Security/blob/master/Core/Authentication/AuthenticationProviderManager.php#L96

5, So in the next request it is going to be empty.

6, I suggest instead of using the $passwordValid = $this->encoder->isPasswordValid($user, $token->getCredentials()); DaoAuthenticationProvider which deal very well this situation:
https://github.com/symfony/security-core/blob/master/Authentication/Provider/DaoAuthenticationProvider.php#L57

[wouterj]

Ah, I think I get what you mean (and what's wrong with the example code). Thanks for debugging & the detailed description!

So if I understand you correctly, you propose this change?

-        $passwordValid = $this->encoder->isPasswordValid($user, $token->getCredentials());
+        $currentUser = $token->getUser();
+
+        if ($currentUser instanceof UserInterface) {
+            if ($currentUser->getPassword() !== $user->getPassword()) {
+                throw new BadCredentialsException('The credentials were changed from another session.');
+            }
+        } else {
+            if ('' === ($presentedPassword = $token->getCredentials())) {
+                throw new BadCredentialsException('The presented password cannot be empty.');
+            }
+            if (!$this->encoderFactory->getEncoder($user)->isPasswordValid($user->getPassword(), $presentedPassword, $user->getSalt())) {
+                throw new BadCredentialsException('The presented password is invalid.');
+            }
+        }

[stof]

@burci UserInterface::eraseCredentials is not meant to remove the hashed password from the user (removing it will indeed break things as Symfony needs it to compare it later). It is meant to remove plain-text credentials in case you stored them in a property of the user at some point.

[burci]

@wouterj yes, something like this. In my code I use DaoAuthenticationProvider in order not to reimplement this logic, but maybe using it is not clear enought for this example...

@stof The user object is not touched by the eraseCredentials, only the token.

In the current https://github.com/symfony/symfony-docs/blob/master/cookbook/security/custom_password_authenticator.rst password is checked with:
$passwordValid = $this->encoder->isPasswordValid($user, $token->getCredentials()); function call.

This means that the code tries to check the user password always against $token->getCredentials() which is going to be null because AuthenticationProviderManager will erase it right after the first successful authentication.

In the documentation there is a part mentioning "Ultimately, your job is to return a new token object" which would make only sense if AuthenticationProviderManager would not eraseCredentials on the result token, but on the original one: https://github.com/symfony/Security/blob/master/Core/Authentication/AuthenticationProviderManager.php#L96

[stof]

@burci eraseCredentials in the token just calls the same method on the user.

[burci]

@stof when i wrote "user object is not touched" i mean it does not change it, in my implementation the function is empty:

public function eraseCredentials()
{
}

Sorry if I was not clear enough. This is a real documentation error, and it is not related to my local implementation.

As @wouterj already pointed out, the problem is that eraseCredentials is going to set the token's credential to null https://github.com/symfony/Security/blob/master/Core/Authentication/Token/UsernamePasswordToken.php#L88 so the

$passwordValid = $this->encoder->isPasswordValid($user, $token->getCredentials());

is only enough to check the token at login time, in the next request $passwordValid is going to be false.

This is why DaoAuthenticationProvider also checks against the $token->getUser(), i think this lines describes the situation clearly:

https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Core/Authentication/Provider/DaoAuthenticationProvider.php#L57

[adrcal]

I just encountered exactly the same thing by implementing the recipe from http://symfony.com/doc/2.4/cookbook/security/custom_authentication_provider.html. The authenticate metod is being called twice, and the second time the token's credentials indeed are null and hence an infinite loop.

The scenario I encountered is detailed step by step on this question of over 1 year old:

http://stackoverflow.com/questions/23390327/simple-form-symfony2-firewall-redirection

So this seems like a documentation error for me two.