Skip to content
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

Email showing incorrect hour(s) #119

Closed
fd6130 opened this issue Jun 13, 2020 · 15 comments · Fixed by #135
Closed

Email showing incorrect hour(s) #119

fd6130 opened this issue Jun 13, 2020 · 15 comments · Fixed by #135
Labels
Bug Bug Fix maker bundle related Cause is related to maker bundle

Comments

@fd6130
Copy link

fd6130 commented Jun 13, 2020

There is a code in .html.twig to show the expire hour(s):

This link will expire in {{ tokenLifetime|date('g') }} hour(s)..

The tokenLifetime is 3600 and i expect it show as 1 hour(s) but it display as

This link will expire in 9 hour(s)..

in my email.

My requested_at and expires_at in database are

requested_at expires_at
2020-06-13 15:23:00 2020-06-13 16:23:00
@jrushlow
Copy link
Collaborator

@fd6130 Are you by any chance setting either the lifetime and/or the throttle_limit as described in https://github.com/symfonycasts/reset-password-bundle#configuration ?

@jrushlow jrushlow added the question Further information is requested label Jun 13, 2020
@fd6130
Copy link
Author

fd6130 commented Jun 13, 2020

@fd6130 Are you by any chance setting either the lifetime and/or the throttle_limit as described in https://github.com/symfonycasts/reset-password-bundle#configuration ?

Hi, i'm using the default one and this is how my reset_password.yaml looks like:

symfonycasts_reset_password:
    request_password_repository: App\Repository\ResetPasswordRequestRepository

@fd6130
Copy link
Author

fd6130 commented Jun 13, 2020

@jrushlow I've tried to add lifetime and/or the throttle_limit with 3600 value as described in docs but it is still the same.

This bundle https://github.com/SymfonyCasts/verify-email-bundle also have the same issue to me.

The weird thing is, reset-password email display as 9 hour(s) while verify-email display as 12 hour(s). Both lifetime are 3600 by default.

This is verify-email-bundle email twig result:

Hi! Please confirm your email!
Please confirm your email address by clicking the following link:

Confirm my Email. This link will expire in 12 hour(s).

Cheers!

And this is reset-password-bundle email twig result:

This link will expire in 9 hour(s)..

Cheers!

@fd6130
Copy link
Author

fd6130 commented Jun 13, 2020

Forgot to mention this, i didn't use the Form and Session class generate by maker. I have modified the controller to use POST request with json response.

@jrushlow
Copy link
Collaborator

jrushlow commented Jun 13, 2020

I'm trying to hunt down the cause of the issue and I'm scratching my head. Behind the scenes, we set 3600 via a constructor argument to ResetPasswordHelper::resetRequestLifetime. The resetRequestLifetime parameter is not mutated in the helper. In the ResetPasswordController we call $this->resetPasswordHelper->getTokenLifetime() and pass that return to the twig template as tokenLifetime.

So the only way for tokenLifetime to be changed is either by:
a) passing a different value to ResetPasswordHelper::__constructor(....., $resetRequestLifetime....)
or
b) passing a different value to tokenLifetime in the controller for the email.html.twig template. e.g.

// ResetPasswordController  // This is what should be taking place..

private function processSendingPasswordResetEmail(......)
{
......
$email = (new TemplatedEmail())
            ......... // ->from() ->to() etc....
            ->htmlTemplate('reset_password/email.html.twig')
            ->context([
                'resetToken' => $resetToken,
                'tokenLifetime' => $this->resetPasswordHelper->getTokenLifetime(),
            ])
;

Having said that, if neither of the above have been changed - than theoretically something in twig is manipulating tokenLifetime. I'll dig into this some more and try to figure out what is going on. In the mean time, if you could verify that your config is set correctly by:

developer@a35baaf414e0:/var/htdocs$ bin/console debug:config symfonycasts_reset_password

Current configuration for extension with alias "symfonycasts_reset_password"
============================================================================

symfonycasts_reset_password:
    request_password_repository: App\Repository\ResetPasswordRequestRepository
    lifetime: 3600
    throttle_limit: 3600
    enable_garbage_collection: true

Also as possible, but unlikely solution, clear out all Symfony caching and see if the emails still display the wrong value.

As a temporary fix, you can modify the reset_password/email.html.twig template to:

<p>
    To reset your password, please visit
    <a href="{{ url('app_reset_password', {token: resetToken.token}) }}">here</a>
    -This link will expire in {{ tokenLifetime|date('g') }} hour(s)..
    +This link will expire in 1 hour(s)..
</p>

Hopefully I'll be able to reproduce this locally and push up a fix pretty quick..

@kbond
Copy link
Contributor

kbond commented Jun 13, 2020

Could this be a timezone issue?

@jrushlow
Copy link
Collaborator

I don’t think so as PHP’s DateTime doesn’t become aware of the lifetime (in this context) until it is called within the twig template. And even then date() is only getting 3600 as an arg.

@fd6130 fd6130 closed this as completed Jun 13, 2020
@fd6130
Copy link
Author

fd6130 commented Jun 13, 2020

Could this be a timezone issue?

Not sure. I'm using Asia/Kuala_Lumpur timezone.

Lol sorry i just accidentally pressed close issues in mobile chrome.😅

@fd6130 fd6130 reopened this Jun 13, 2020
@fd6130
Copy link
Author

fd6130 commented Jun 13, 2020

Actually guys, what is the result of tokenLifeTime | date('g') in your twig? Assume the tokenLifeTime is 3600.

Is it same as datetime in php code:

$date = ( new \DateTime(3600) )->format('g');

?

@jrushlow
Copy link
Collaborator

@fd6130 Would you be able to modify the email templates as described in symfony/maker-bundle#632 to verify that solution solves your issue?

I believe the issue is caused by a timezone being set in Twig or in PHP. While both the ResetPasswordBundle & VerifyEmailBundle are immune to timezone changes in regards to lifetime values - the templates generated by MakerBundle are not.

I've created a short term fix in the aforementioned PR that should take care of this. When I get some free time, I'll come up with a better solution to convert the token lifetime to a human readable format. Using the date filter in the twig templates was a quick solution to the problem early on.

@fd6130
Copy link
Author

fd6130 commented Jun 14, 2020

@fd6130 Would you be able to modify the email templates as described in symfony/maker-bundle#632 to verify that solution solves your issue?

I believe the issue is caused by a timezone being set in Twig or in PHP. While both the ResetPasswordBundle & VerifyEmailBundle are immune to timezone changes in regards to lifetime values - the templates generated by MakerBundle are not.

I've created a short term fix in the aforementioned PR that should take care of this. When I get some free time, I'll come up with a better solution to convert the token lifetime to a human readable format. Using the date filter in the twig templates was a quick solution to the problem early on.

Interesting. After apply the ... | date('g', 'UTC') filter to both of the email template(reset password & verify email):

This link will expire in {{ tokenLifetime|date('g', 'UTC') }} hour(s)..
This link will expire in {{ expiresAt|date('g', 'UTC') }} hour(s).

And the result is different:
This link will expire in 1 hour(s).
This link will expire in 4 hour(s).

It seems the first one is ok while the other one still having issue.

@j-herden
Copy link

Same bug here.
I can confirm that tokenLifetime | date('g', 'UTC') has fixed it for me too.

j-herden added a commit to j-herden/ff-atemschutz that referenced this issue Aug 16, 2020
@edgargalvezr
Copy link

Putting tokenLifetime|date('G', 'UTC') works perfectly, it should be implemented when executing php bin\console make:reset-password

@weaverryan
Copy link
Contributor

There's a PR here - https://github.com/symfony/maker-bundle/pull/632/files - but it was tricky enough, that I was waiting for @jrushlow to verify that it works. But there are enough +1 here to support that.

But, it seems like we should use G instead of g (as suggested by @edgargalvezr)? I'm guessing it will make a difference if you set a lifetime that's more than 12 hours but less than 24?

@noiob
Copy link

noiob commented Nov 2, 2020

Hi, I ran into this error earlier and I don't think tokenLifetime|date('g', 'UTC') or tokenLifetime|date('G', 'UTC') solves the issue very well.

The biggest problem here is that you're using twig's date to format something as a date/time, when the thing you're formatting (tokenLifetime) is a time interval. I guess it works if you set the time zone, but it's a hack, and it caused this bug in the first place. It also wraps around for values over 23 hours.

Twig date can actually take DateInterval objects, but for reasons those won't convert seconds into hours.

The simplest solution I've found that doesn't wrap after 24 hours is tokenLifetime//3600. Note the integer division (we don't want decimal hours here 😉)

Oh, and you might actually want to use the server's timezone on expiresAt, since that seems to be an actual date/time?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Bug Fix maker bundle related Cause is related to maker bundle
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants