From 1ec478f607805ebd88ba503d2b89cf9303ecfda7 Mon Sep 17 00:00:00 2001 From: Jesse Rushlow Date: Fri, 1 Mar 2024 07:35:27 -0500 Subject: [PATCH 1/5] [persistence] add ability to remove ResetPasswordRequest objects programmatically --- .../FakeResetPasswordInternalRepository.php | 5 +++ .../ResetPasswordRequestRepositoryTrait.php | 28 +++++++++++++ ...esetPasswordRequestRepositoryInterface.php | 2 + .../ResetPasswordRequestRepositoryTest.php | 39 +++++++++++++++++++ ...akeResetPasswordInternalRepositoryTest.php | 1 + 5 files changed, 75 insertions(+) diff --git a/src/Persistence/Fake/FakeResetPasswordInternalRepository.php b/src/Persistence/Fake/FakeResetPasswordInternalRepository.php index d146b8f7..adf6b773 100644 --- a/src/Persistence/Fake/FakeResetPasswordInternalRepository.php +++ b/src/Persistence/Fake/FakeResetPasswordInternalRepository.php @@ -60,4 +60,9 @@ public function removeExpiredResetPasswordRequests(): int { throw new FakeRepositoryException(); } + + public function removeRequests(?object $user = null): void + { + throw new FakeRepositoryException(); + } } diff --git a/src/Persistence/Repository/ResetPasswordRequestRepositoryTrait.php b/src/Persistence/Repository/ResetPasswordRequestRepositoryTrait.php index 52dfcaae..9d93b1e4 100644 --- a/src/Persistence/Repository/ResetPasswordRequestRepositoryTrait.php +++ b/src/Persistence/Repository/ResetPasswordRequestRepositoryTrait.php @@ -82,4 +82,32 @@ public function removeExpiredResetPasswordRequests(): int return $query->execute(); } + + /** + * Remove ResetPasswordRequest objects from persistence. + * + * Warning - This is a destructive operation. Calling this method + * may have undesired consequences for users who have valid + * ResetPasswordRequests but have not "checked their email" yet. + * + * @TODO _ @legacy This method is useful for something..... + * + * If a "user" object is passed, only the requests for that user + * will be removed. + */ + public function removeRequests(?object $user = null): void + { + $query = $this->createQueryBuilder('t') + ->delete() + ; + + if (null !== $user) { + $query + ->where('t.user = :user') + ->setParameter('user', $user) + ; + } + + $query->getQuery()->execute(); + } } diff --git a/src/Persistence/ResetPasswordRequestRepositoryInterface.php b/src/Persistence/ResetPasswordRequestRepositoryInterface.php index 25818c1f..173c27cd 100644 --- a/src/Persistence/ResetPasswordRequestRepositoryInterface.php +++ b/src/Persistence/ResetPasswordRequestRepositoryInterface.php @@ -14,6 +14,8 @@ /** * @author Jesse Rushlow * @author Ryan Weaver + * + * @method void removeRequests(?object $user = null) Remove ResetPasswordRequest objects from persistence. */ interface ResetPasswordRequestRepositoryInterface { diff --git a/tests/FunctionalTests/Persistence/ResetPasswordRequestRepositoryTest.php b/tests/FunctionalTests/Persistence/ResetPasswordRequestRepositoryTest.php index 8a4034ef..312f2443 100644 --- a/tests/FunctionalTests/Persistence/ResetPasswordRequestRepositoryTest.php +++ b/tests/FunctionalTests/Persistence/ResetPasswordRequestRepositoryTest.php @@ -208,6 +208,45 @@ public function testRemovedExpiredResetPasswordRequestsOnlyRemovedExpiredRequest self::assertSame($futureFixture, $result[0]); } + public function testRemoveRequestsRemovesAllRequestsFromPersistence(): void + { + $this->manager->persist(new ResetPasswordTestFixtureRequest()); + $this->manager->persist(new ResetPasswordTestFixtureRequest()); + + $this->manager->flush(); + + $this->repository->removeRequests(); + + self::assertCount(0, $this->repository->findAll()); + } + + public function testRemoveRequestsRemovesAllRequestsForASingleUser(): void + { + $this->manager->persist($userFixture = new ResetPasswordTestFixtureUser()); + $requestFixtures = [new ResetPasswordTestFixtureRequest(), new ResetPasswordTestFixtureRequest()]; + + foreach ($requestFixtures as $fixture) { + $fixture->user = $userFixture; + + $this->manager->persist($fixture); + } + + $this->manager->persist($differentUserFixture = new ResetPasswordTestFixtureUser()); + + $existingRequestFixture = new ResetPasswordTestFixtureRequest(); + $existingRequestFixture->user = $differentUserFixture; + + $this->manager->persist($existingRequestFixture); + $this->manager->flush(); + + self::assertCount(3, $this->repository->findAll()); + + $this->repository->removeRequests($userFixture); + + self::assertCount(1, $result = $this->repository->findAll()); + self::assertSame($existingRequestFixture, $result[0]); + } + private function configureDatabase(): void { $metaData = $this->manager->getMetadataFactory(); diff --git a/tests/UnitTests/Persistence/FakeResetPasswordInternalRepositoryTest.php b/tests/UnitTests/Persistence/FakeResetPasswordInternalRepositoryTest.php index cf505f79..5d2a71cb 100644 --- a/tests/UnitTests/Persistence/FakeResetPasswordInternalRepositoryTest.php +++ b/tests/UnitTests/Persistence/FakeResetPasswordInternalRepositoryTest.php @@ -28,6 +28,7 @@ public function methodDataProvider(): \Generator yield ['findResetPasswordRequest', ['']]; yield ['getMostRecentNonExpiredRequestDate', [new \stdClass()]]; yield ['removeResetPasswordRequest', [$this->createMock(ResetPasswordRequestInterface::class)]]; + yield ['removeRequests', []]; } /** From 239a9ad083237be6165150d3c63861e8d1f33ded Mon Sep 17 00:00:00 2001 From: Jesse Rushlow Date: Fri, 1 Mar 2024 11:43:06 -0500 Subject: [PATCH 2/5] document purging requests --- README.md | 23 +++++++++++++++++++ .../ResetPasswordRequestRepositoryTrait.php | 4 ++-- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 9284fc45..20415c6c 100644 --- a/README.md +++ b/README.md @@ -105,6 +105,29 @@ _Optional_ - Defaults to `true` Enable or disable the Reset Password Cleaner which handles expired reset password requests that may have been left in persistence. +## Advanced Usage + +### Purging `ResetPasswordRequest` objects from persistence + +The `ResetPasswordRequestRepositoryInterface::removeRequests()` method, which is +implemented in the +[ResetPasswordRequestRepositoryTrait](https://github.com/SymfonyCasts/reset-password-bundle/blob/main/src/Persistence/Repository/ResetPasswordRequestRepositoryTrait.php), +can be used to remove request objects from persistence for a single user or all +users. This differs from the +[garbage collection mechanism](https://github.com/SymfonyCasts/reset-password-bundle/blob/df64d82cca2ee371da5e8c03c227457069ae663e/src/Persistence/Repository/ResetPasswordRequestRepositoryTrait.php#L73) +which only removes _expired_ request objects for _all_ users automatically. + +Typically, you'd call this method when you need to remove request object(s) for +a user who changed their email address due to suspicious activity and potentially +has valid request objects in persistence with their "old" compromised email address. +In such cases, pass the `user` object to `removeRequest()`. + +Passing `null` or omitting a `user` object will remove all request objects from +persistence - even if any of those requests have not expired. + +This method relies on the user objects `primary key` being `immutable`. +E.g. `User::id = UUID` not something like `User::id = 'john@example.com'` + ## Support Feel free to open an issue for questions, problems, or suggestions with our bundle. diff --git a/src/Persistence/Repository/ResetPasswordRequestRepositoryTrait.php b/src/Persistence/Repository/ResetPasswordRequestRepositoryTrait.php index 9d93b1e4..87e5d60f 100644 --- a/src/Persistence/Repository/ResetPasswordRequestRepositoryTrait.php +++ b/src/Persistence/Repository/ResetPasswordRequestRepositoryTrait.php @@ -90,10 +90,10 @@ public function removeExpiredResetPasswordRequests(): int * may have undesired consequences for users who have valid * ResetPasswordRequests but have not "checked their email" yet. * - * @TODO _ @legacy This method is useful for something..... - * * If a "user" object is passed, only the requests for that user * will be removed. + * + * @see https://github.com/SymfonyCasts/reset-password-bundle?tab=readme-ov-file#advanced-usage */ public function removeRequests(?object $user = null): void { From 2bc192a69c638605a4a6ac07e7140554d2e31407 Mon Sep 17 00:00:00 2001 From: Jesse Rushlow Date: Tue, 5 Mar 2024 03:34:07 -0500 Subject: [PATCH 3/5] require a user object --- README.md | 42 +++++++++++++++---- .../FakeResetPasswordInternalRepository.php | 2 +- .../ResetPasswordRequestRepositoryTrait.php | 16 ++----- ...esetPasswordRequestRepositoryInterface.php | 2 +- .../ResetPasswordRequestRepositoryTest.php | 12 ------ ...akeResetPasswordInternalRepositoryTest.php | 2 +- 6 files changed, 42 insertions(+), 34 deletions(-) diff --git a/README.md b/README.md index 20415c6c..4117f738 100644 --- a/README.md +++ b/README.md @@ -112,21 +112,49 @@ requests that may have been left in persistence. The `ResetPasswordRequestRepositoryInterface::removeRequests()` method, which is implemented in the [ResetPasswordRequestRepositoryTrait](https://github.com/SymfonyCasts/reset-password-bundle/blob/main/src/Persistence/Repository/ResetPasswordRequestRepositoryTrait.php), -can be used to remove request objects from persistence for a single user or all -users. This differs from the +can be used to remove all request objects from persistence for a single user. This +differs from the [garbage collection mechanism](https://github.com/SymfonyCasts/reset-password-bundle/blob/df64d82cca2ee371da5e8c03c227457069ae663e/src/Persistence/Repository/ResetPasswordRequestRepositoryTrait.php#L73) which only removes _expired_ request objects for _all_ users automatically. Typically, you'd call this method when you need to remove request object(s) for a user who changed their email address due to suspicious activity and potentially has valid request objects in persistence with their "old" compromised email address. -In such cases, pass the `user` object to `removeRequest()`. - -Passing `null` or omitting a `user` object will remove all request objects from -persistence - even if any of those requests have not expired. This method relies on the user objects `primary key` being `immutable`. -E.g. `User::id = UUID` not something like `User::id = 'john@example.com'` +E.g. `User::id = UUID` not something like `User::id = 'john@example.com'`. + +```php +// ProfileController + +#[Route(path: '/profile/{id}', name: 'app_update_profile', methods: ['GET', 'POST'])] +public function profile(Request $request, User $user, ResetPasswordRequestRepositoryInterface $repository): Response +{ + $form = $this->createFormBuilder($user) + ->add('email', EmailType::class) + ->add('save', SubmitType::class, ['label' => 'Save Profile']) + ->getForm() + ; + + $form->handleRequest($request); + + if ($form->isSubmitted() && $form->isValid()) { + $newEmail = $form->get('email')->getData(); + + if ($newEmail !== $user->getEmail()) { + // The user changed their email address. + // Remove any old reset requests for the user. + $repository->removeRequests($user); + } + + // Persist the user object... + + return $this->render('success.html.twig'); + } + + return $this->render('profile.html.twig', ['form' => $form]); +} +``` ## Support diff --git a/src/Persistence/Fake/FakeResetPasswordInternalRepository.php b/src/Persistence/Fake/FakeResetPasswordInternalRepository.php index adf6b773..ec9d465e 100644 --- a/src/Persistence/Fake/FakeResetPasswordInternalRepository.php +++ b/src/Persistence/Fake/FakeResetPasswordInternalRepository.php @@ -61,7 +61,7 @@ public function removeExpiredResetPasswordRequests(): int throw new FakeRepositoryException(); } - public function removeRequests(?object $user = null): void + public function removeRequests(object $user): void { throw new FakeRepositoryException(); } diff --git a/src/Persistence/Repository/ResetPasswordRequestRepositoryTrait.php b/src/Persistence/Repository/ResetPasswordRequestRepositoryTrait.php index 87e5d60f..f446f5cf 100644 --- a/src/Persistence/Repository/ResetPasswordRequestRepositoryTrait.php +++ b/src/Persistence/Repository/ResetPasswordRequestRepositoryTrait.php @@ -84,30 +84,22 @@ public function removeExpiredResetPasswordRequests(): int } /** - * Remove ResetPasswordRequest objects from persistence. + * Remove a users ResetPasswordRequest objects from persistence. * * Warning - This is a destructive operation. Calling this method * may have undesired consequences for users who have valid * ResetPasswordRequests but have not "checked their email" yet. * - * If a "user" object is passed, only the requests for that user - * will be removed. - * * @see https://github.com/SymfonyCasts/reset-password-bundle?tab=readme-ov-file#advanced-usage */ - public function removeRequests(?object $user = null): void + public function removeRequests(object $user): void { $query = $this->createQueryBuilder('t') ->delete() + ->where('t.user = :user') + ->setParameter('user', $user) ; - if (null !== $user) { - $query - ->where('t.user = :user') - ->setParameter('user', $user) - ; - } - $query->getQuery()->execute(); } } diff --git a/src/Persistence/ResetPasswordRequestRepositoryInterface.php b/src/Persistence/ResetPasswordRequestRepositoryInterface.php index 173c27cd..ede76311 100644 --- a/src/Persistence/ResetPasswordRequestRepositoryInterface.php +++ b/src/Persistence/ResetPasswordRequestRepositoryInterface.php @@ -15,7 +15,7 @@ * @author Jesse Rushlow * @author Ryan Weaver * - * @method void removeRequests(?object $user = null) Remove ResetPasswordRequest objects from persistence. + * @method void removeRequests(object $user) Remove a users ResetPasswordRequest objects from persistence. */ interface ResetPasswordRequestRepositoryInterface { diff --git a/tests/FunctionalTests/Persistence/ResetPasswordRequestRepositoryTest.php b/tests/FunctionalTests/Persistence/ResetPasswordRequestRepositoryTest.php index 312f2443..1cbe9314 100644 --- a/tests/FunctionalTests/Persistence/ResetPasswordRequestRepositoryTest.php +++ b/tests/FunctionalTests/Persistence/ResetPasswordRequestRepositoryTest.php @@ -208,18 +208,6 @@ public function testRemovedExpiredResetPasswordRequestsOnlyRemovedExpiredRequest self::assertSame($futureFixture, $result[0]); } - public function testRemoveRequestsRemovesAllRequestsFromPersistence(): void - { - $this->manager->persist(new ResetPasswordTestFixtureRequest()); - $this->manager->persist(new ResetPasswordTestFixtureRequest()); - - $this->manager->flush(); - - $this->repository->removeRequests(); - - self::assertCount(0, $this->repository->findAll()); - } - public function testRemoveRequestsRemovesAllRequestsForASingleUser(): void { $this->manager->persist($userFixture = new ResetPasswordTestFixtureUser()); diff --git a/tests/UnitTests/Persistence/FakeResetPasswordInternalRepositoryTest.php b/tests/UnitTests/Persistence/FakeResetPasswordInternalRepositoryTest.php index 5d2a71cb..0246202f 100644 --- a/tests/UnitTests/Persistence/FakeResetPasswordInternalRepositoryTest.php +++ b/tests/UnitTests/Persistence/FakeResetPasswordInternalRepositoryTest.php @@ -28,7 +28,7 @@ public function methodDataProvider(): \Generator yield ['findResetPasswordRequest', ['']]; yield ['getMostRecentNonExpiredRequestDate', [new \stdClass()]]; yield ['removeResetPasswordRequest', [$this->createMock(ResetPasswordRequestInterface::class)]]; - yield ['removeRequests', []]; + yield ['removeRequests', [new \stdClass()]]; } /** From 0cca2680c003b6b133e532c10ed92fdab67540a4 Mon Sep 17 00:00:00 2001 From: Jesse Rushlow Date: Tue, 5 Mar 2024 15:25:24 -0500 Subject: [PATCH 4/5] fix up the example --- README.md | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 4117f738..01abfc11 100644 --- a/README.md +++ b/README.md @@ -130,6 +130,8 @@ E.g. `User::id = UUID` not something like `User::id = 'john@example.com'`. #[Route(path: '/profile/{id}', name: 'app_update_profile', methods: ['GET', 'POST'])] public function profile(Request $request, User $user, ResetPasswordRequestRepositoryInterface $repository): Response { + $originalEmail = $user->getEmail(); + $form = $this->createFormBuilder($user) ->add('email', EmailType::class) ->add('save', SubmitType::class, ['label' => 'Save Profile']) @@ -139,17 +141,13 @@ public function profile(Request $request, User $user, ResetPasswordRequestReposi $form->handleRequest($request); if ($form->isSubmitted() && $form->isValid()) { - $newEmail = $form->get('email')->getData(); - - if ($newEmail !== $user->getEmail()) { + if ($originalEmail !== $user->getEmail()) { // The user changed their email address. // Remove any old reset requests for the user. $repository->removeRequests($user); } - // Persist the user object... - - return $this->render('success.html.twig'); + // Persist the user object and redirect... } return $this->render('profile.html.twig', ['form' => $form]); From f69d28cd6601e12506feeb54f968b77b5b542b5f Mon Sep 17 00:00:00 2001 From: Jesse Rushlow Date: Tue, 5 Mar 2024 15:28:47 -0500 Subject: [PATCH 5/5] remove tip --- README.md | 3 --- 1 file changed, 3 deletions(-) diff --git a/README.md b/README.md index 01abfc11..540c2922 100644 --- a/README.md +++ b/README.md @@ -121,9 +121,6 @@ Typically, you'd call this method when you need to remove request object(s) for a user who changed their email address due to suspicious activity and potentially has valid request objects in persistence with their "old" compromised email address. -This method relies on the user objects `primary key` being `immutable`. -E.g. `User::id = UUID` not something like `User::id = 'john@example.com'`. - ```php // ProfileController