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

Fix resetting of PasswordResetTokenEmailsSent Value after Expiring #7438

Open
wants to merge 12 commits into
base: 2.5
Choose a base branch
from

Conversation

TimonStadelmann
Copy link

@TimonStadelmann TimonStadelmann commented May 22, 2024

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets fixes #7037
License MIT

What's in this PR?

Fix check of passwordResetTokenExpiresAt by moving it to the calling function and dividing it into two checks. So that if the token is expired the passwordResetTokenEmailsSent is reset to 0. Otherwise it's going to be checked whether the maxNumberEmails was reached or not, and acted upon that accordingly.

Why?

"Reset password does not work properly.
If someone makes a reset password request, request works and "passwordResetTokenEmailsSent" goes up to 3.
Even 48h after, passwordResetTokenEmailsSent stays at 3, so reset emails are not sent.
BUT, this number is never reset to 0, even if time interval (24h) is done."

To Do

  • Gather feedback for my changes
  • Change target branch to 2.5
  • Fix lint
  • Add test case

@alexander-schranz alexander-schranz added the Bug Error or unexpected behavior of already existing functionality label May 22, 2024
@alexander-schranz alexander-schranz changed the title Fixed passwordResetTokenExpiresAt-check Fix reseting of PasswordResetTokenEmailsSent Value after Expiring May 22, 2024
@alexander-schranz alexander-schranz changed the title Fix reseting of PasswordResetTokenEmailsSent Value after Expiring Fix resetting of PasswordResetTokenEmailsSent Value after Expiring May 22, 2024
@alexander-schranz
Copy link
Member

alexander-schranz commented May 22, 2024

Thx for the pull request. There should be a test case added which uses a expired token with a count limit to check if it get correctly reseted. Also as a bugfix this merge request should target the 2.5 branch, you can do that via:

git rebase -i origin/2.6 --onto origin/2.5

git push yourremote yourbranch --force

if origin is your fork you can rebase to sulu 2.5 via:

git remote add sulu git@github.com:sulu/sulu.git
git fetch sulu

git rebase -i sulu/2.6 --onto sulu/2.5

git push yourremote yourbranch --force

And then change the target branch here.

@TimonStadelmann TimonStadelmann force-pushed the bugfix/password-reset-token-expiresat-check branch from e035f8f to ca6800d Compare May 22, 2024 14:16
@TimonStadelmann TimonStadelmann changed the base branch from 2.6 to 2.5 May 22, 2024 14:16
@TimonStadelmann TimonStadelmann force-pushed the bugfix/password-reset-token-expiresat-check branch from 446c168 to dd5aa98 Compare June 6, 2024 07:05
@TimonStadelmann TimonStadelmann force-pushed the bugfix/password-reset-token-expiresat-check branch from dd5aa98 to aa1614a Compare June 7, 2024 23:29
@alexander-schranz
Copy link
Member

Let us know if you need any help here or have questions.

@TimonStadelmann
Copy link
Author

Let us know if you need any help here or have questions.

Yeah please, I don't really understand why the tests never work. Am I missing something?

@alexander-schranz
Copy link
Member

alexander-schranz commented Jun 27, 2024

  1. Sulu\Bundle\SecurityBundle\Tests\Functional\Controller\ResettingControllerTest::testResetCounterAfterTokenExpiration
    InvalidArgumentException: Collector "swiftmailer" does not exist.

Swiftmailer is only used on lowest versions newer version using the Symfony Mailer so you need bridge that inside the test. Normally the LegacyResettingControllerTest runs for swiftmailer I think and the other for Symfony Mailer which has its own collector.

octrine\ORM\ORMInvalidArgumentException: A new entity was found through the relationship 'Sulu\Bundle\SecurityBundle\Entity\User#contact' that was not configured to cascade persist operations for entity: Sulu\Bundle\ContactBundle\Entity\Contact@00000000255f63700000000072e9132c. To solve this issue: Either explicitly call EntityManager#persist() on this unknown entity or configure cascade persist this association in the mapping for example @manytoone(..,cascade={"persist"}). If you cannot find out which entity causes the problem implement 'Sulu\Bundle\ContactBundle\Entity\Contact#__toString()' to get a clue.

Is mostly an issue of test fixtures vs test client data. Try to clear the entityManager after you created the fixtures before doing the client request.

…com:TimonStadelmann/sulu into bugfix/password-reset-token-expiresat-check
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Error or unexpected behavior of already existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reset password not working properly
2 participants