add ability to generate a fake reset token

[jrushlow]

Now that we persist the ResetPasswordToken object in the users session when using make:reset-password - we need a convenient way to generate a fake token that is only persisted in the session if reset request is made on a non-existent user.

See symfony/maker-bundle#808

[jellynoone]

Hi, again this also closes #154 . I'm just curious what was wrong with the previous way of doing things?

The way I see this library being implemented in the maker bundle it will still have to use the ->getTokenFromSession not the new-ish getTokenObjectFromSession and these two might look similar but mean very different things. The getTokenObjectFromSession is the token generated on a valid form submission and should never be relied upon in further authentication until the storeTokenInSession is called to save the user provided token.

What is the function of getTokenObjectFromSession anyway, the way I see it, it is essentially a more sophisticated boolean check we had before 1.3, or am I missing something? Do you agree that is actually quite hard to follow what all the methods in the ResetPasswordControllerTrait are supposed to be doing?

[jrushlow]

Howdy @jellynoone

Prior to MakerBundle 1.28 - we were unable to correctly display the expiration time of the request to the user. As we needed to pass multiple values from the ResetPasswordToken to the template, it made more sense to pass the entire token object in the session vs multiple calls to session setters and getters for individual properties.

As a result, we deprecated the canCheckEmail() method which will be removed in the future, and added the getter / setter methods needed to store the reset token in the session. As these new methods are required to meet our needs in MakerBundle, it makes no sense to keep the old canCheck method.

To fix our double redirect issue, we are now implementing a generateFakeToken() method that will be used to store a fake token object in the session. It is undetermined yet if this will live in the ResetPasswordHelperInterface or the trait yet as there are pro's and con's to both.

Do you agree that is actually quite hard to follow what all the methods in the ResetPasswordControllerTrait are supposed to be doing?

Implementing sound and secure password resets is tricky and early on we decided to abstract as much of the logic out of maker as possible to keep the generated code as clean as possible. The ResetPasswordControllerTrait exists primarily for maker bundle and as a result of that design decision. Your feedback is appreciated and we'll definitely take it into consideration when decisions are made in the future. As always, using the trait is not a requirement and PR's are always welcome for improvements to the bundle.

[jellynoone]

Ok, makes sense. Thank you for the explanation.

[weaverryan]

Yes to this PR... but I have a different idea on where we would use it. We make no changes to processSendingPasswordResetEmail() as suggested in symfony/maker-bundle#808 (comment).

Instead, we make checkEmail() accessible to anyone. Who cares if someone navigates to it directly? In THAT spot we would create the fake token:

    /**
     * Confirmation page after a user has requested a password reset.
     *
     * @Route("/check-email", name="app_check_email")
     */
-   public function checkEmail(): Response
+   public function checkEmail(ResetPasswordHelperInterface $helper): Response
    {
-       // We prevent users from directly accessing this page
-       if (null === ($resetToken = $this->getTokenObjectFromSession())) {
-            return $this->redirectToRoute('app_forgot_password_request');
+       $resetToken = $this->getTokenObjectFromSession();
+       if (null === $resetToken) {
+           // make a fake token so we don't expose if an email was really found
+           $resetToken = $helper->generateFakeResetToken();
        }

        return $this->render('reset_password/check_email.html.twig', [
            'resetToken' => $resetToken,
        ]);
    }

We could also tweak the language in the check_email.html.twig template:

- An email has been sent that contains a link that you can click to reset your password.
+ If an account matching your email exists, then it an email was just sent that contains a link that you can use to reset your password.

[weaverryan]

Clarify usage (so add this):

Use this to generate a fake token so that you can, for example, show a "reset confirmation email sent"
page that includes a valid "expiration date", even if the email was not actually found (and so, a true
ResetPasswordToken was not actually created).

[jrushlow]

done

[jrushlow]

Instead, we make checkEmail() accessible to anyone. Who cares if someone navigates to it directly? In THAT spot we would create the fake token:

I actually forgot about this PR and did this very thing in a project the other day. Works great!

We should be ready for review again.

Side note - It's worth repeating that when relying on generating a "fake" token if a user is not found, it is technically possible for someone to perform a timing attack to determine if the email provided actually exists. e.g. it takes 2 seconds to generate a real token but only 1 second to generate a fake token. While I do not believe this should be a major concern for most, mitigating this is beyond the scope of this bundle.

[weaverryan]

Why not make this method as close as possible to the real one, to mitigate timing? What Specifically, I'm thinking about running:

reset-password-bundle/src/ResetPasswordHelper.php

Line 75 in 6825b9f

$tokenComponents = $this->tokenGenerator->createToken($expiresAt, $this->repository->getUserIdentifier($user));

.. even if we then don't use it.

[jrushlow]

that would help, but we would need to init a user entity.

what about changing the method signature to public function generateFakeResetToken(object $user = null): ResetPasswordToken then if null !== $user { $this-repo->getUserIdentifier() ?

This would allow you to generate fake tokens w/o needing persistence. But to slim down the timing attack vector, a consumer would have to init and supply an entity if they choose to (not something we could / should do in the bundle itself)

[weaverryan]

Ah, good point. I vote for... let's leave it for later if someone wants it.

[weaverryan]

Thanks @jrushlow!