From 2b31b185e4939e41a58ff3216f3fa214313cc3c4 Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Sat, 30 Jul 2022 13:02:06 +0100 Subject: [PATCH] feat: clear password & email tokens when appropriate (#3567) * test: password tokens are generated and deleted on password change * chore: delete all password tokens when the password is changed * test: email tokens are generated and deleted on email change * test: email tokens are deleted after password reset * chore: delete email tokens after password change * test: password tokens are deleted after email change * chore: delete password tokens after email change * chore: syntactic sugar * chore: unify event listening --- .../Controller/SavePasswordController.php | 3 +- framework/core/src/User/TokensClearer.php | 39 ++++ framework/core/src/User/User.php | 10 + .../core/src/User/UserServiceProvider.php | 1 + .../api/users/PasswordEmailTokensTest.php | 202 ++++++++++++++++++ .../src/integration/BuildsHttpRequests.php | 12 ++ 6 files changed, 265 insertions(+), 2 deletions(-) create mode 100644 framework/core/src/User/TokensClearer.php create mode 100644 framework/core/tests/integration/api/users/PasswordEmailTokensTest.php diff --git a/framework/core/src/Forum/Controller/SavePasswordController.php b/framework/core/src/Forum/Controller/SavePasswordController.php index 2148ff34b..6543db20c 100644 --- a/framework/core/src/Forum/Controller/SavePasswordController.php +++ b/framework/core/src/Forum/Controller/SavePasswordController.php @@ -89,6 +89,7 @@ class SavePasswordController implements RequestHandlerInterface } catch (ValidationException $e) { $request->getAttribute('session')->put('errors', new MessageBag($e->errors())); + // @todo: must return a 422 instead, look into renderable exceptions. return new RedirectResponse($this->url->to('forum')->route('resetPassword', ['token' => $token->token])); } @@ -97,8 +98,6 @@ class SavePasswordController implements RequestHandlerInterface $this->dispatchEventsFor($token->user); - $token->delete(); - $session = $request->getAttribute('session'); $accessToken = SessionAccessToken::generate($token->user->id); $this->authenticator->logIn($session, $accessToken); diff --git a/framework/core/src/User/TokensClearer.php b/framework/core/src/User/TokensClearer.php new file mode 100644 index 000000000..bb7fcb56c --- /dev/null +++ b/framework/core/src/User/TokensClearer.php @@ -0,0 +1,39 @@ +listen([PasswordChanged::class, EmailChanged::class], [$this, 'clearPasswordTokens']); + $events->listen(PasswordChanged::class, [$this, 'clearEmailTokens']); + } + + /** + * @param PasswordChanged|EmailChanged $event + */ + public function clearPasswordTokens($event): void + { + $event->user->passwordTokens()->delete(); + } + + /** + * @param PasswordChanged $event + */ + public function clearEmailTokens($event): void + { + $event->user->emailTokens()->delete(); + } +} diff --git a/framework/core/src/User/User.php b/framework/core/src/User/User.php index 2de8037f5..50725ef5b 100644 --- a/framework/core/src/User/User.php +++ b/framework/core/src/User/User.php @@ -721,6 +721,16 @@ class User extends AbstractModel return $this->hasMany(EmailToken::class); } + /** + * Define the relationship with the user's email tokens. + * + * @return \Illuminate\Database\Eloquent\Relations\HasMany + */ + public function passwordTokens() + { + return $this->hasMany(PasswordToken::class); + } + /** * Define the relationship with the permissions of all of the groups that * the user is in. diff --git a/framework/core/src/User/UserServiceProvider.php b/framework/core/src/User/UserServiceProvider.php index e936a370c..242e6e480 100644 --- a/framework/core/src/User/UserServiceProvider.php +++ b/framework/core/src/User/UserServiceProvider.php @@ -119,6 +119,7 @@ class UserServiceProvider extends AbstractServiceProvider $events->listen(EmailChangeRequested::class, EmailConfirmationMailer::class); $events->subscribe(UserMetadataUpdater::class); + $events->subscribe(TokensClearer::class); User::registerPreference('discloseOnline', 'boolval', true); User::registerPreference('indexProfile', 'boolval', true); diff --git a/framework/core/tests/integration/api/users/PasswordEmailTokensTest.php b/framework/core/tests/integration/api/users/PasswordEmailTokensTest.php new file mode 100644 index 000000000..d2b349511 --- /dev/null +++ b/framework/core/tests/integration/api/users/PasswordEmailTokensTest.php @@ -0,0 +1,202 @@ +prepareDatabase([ + 'users' => [ + $this->normalUser(), + ], + ]); + } + + /** @test */ + public function actor_has_no_tokens_by_default() + { + $this->app(); + + $this->assertEquals(0, PasswordToken::query()->where('user_id', 2)->count()); + $this->assertEquals(0, EmailToken::query()->where('user_id', 2)->count()); + } + + /** @test */ + public function password_tokens_are_generated_when_requesting_password_reset() + { + $response = $this->send( + $this->request('POST', '/api/forgot', [ + 'authenticatedAs' => 2, + 'json' => [ + 'email' => 'normal@machine.local' + ] + ]) + ); + + $this->assertEquals(204, $response->getStatusCode()); + $this->assertEquals(1, PasswordToken::query()->where('user_id', 2)->count()); + } + + /** @test */ + public function password_tokens_are_deleted_after_password_reset() + { + $this->app(); + + // Request password change to generate a token. + $response = $this->send( + $this->request('POST', '/api/forgot', [ + 'authenticatedAs' => 2, + 'json' => [ + 'email' => 'normal@machine.local' + ] + ]) + ); + + // Additional Tokens + PasswordToken::generate(2)->save(); + PasswordToken::generate(2)->save(); + + $this->assertEquals(204, $response->getStatusCode()); + $this->assertEquals(3, PasswordToken::query()->where('user_id', 2)->count()); + + // Use a token to reset password + $response = $this->send( + $request = $this->requestWithCsrfToken( + $this->request('POST', '/reset', [ + 'authenticatedAs' => 2, + ])->withParsedBody([ + 'passwordToken' => PasswordToken::query()->latest()->first()->token, + 'password' => 'new-password', + 'password_confirmation' => 'new-password', + ]) + ) + ); + + $this->assertEquals(302, $response->getStatusCode()); + $this->assertEquals(0, PasswordToken::query()->where('user_id', 2)->count()); + } + + /** @test */ + public function email_tokens_are_generated_when_requesting_email_change() + { + $response = $this->send( + $this->request('PATCH', '/api/users/2', [ + 'authenticatedAs' => 2, + 'json' => [ + 'data' => [ + 'attributes' => [ + 'email' => 'new-normal@machine.local' + ] + ], + 'meta' => [ + 'password' => 'too-obscure' + ] + ] + ]) + ); + + $this->assertEquals(200, $response->getStatusCode()); + $this->assertEquals(1, EmailToken::query()->where('user_id', 2)->count()); + } + + /** @test */ + public function email_tokens_are_deleted_when_confirming_email() + { + $this->app(); + + EmailToken::generate('new-normal2@machine.local', 2)->save(); + EmailToken::generate('new-normal3@machine.local', 2)->save(); + $token = EmailToken::generate('new-normal@machine.local', 2); + $token->save(); + + $response = $this->send( + $this->requestWithCsrfToken( + $this->request('POST', '/confirm/'.$token->token, [ + 'authenticatedAs' => 2 + ]) + ) + ); + + $this->assertEquals(302, $response->getStatusCode()); + $this->assertEquals(0, EmailToken::query()->where('user_id', 2)->count()); + } + + /** @test */ + public function email_tokens_are_deleted_after_password_reset() + { + $this->app(); + + // Request password change to generate a token. + $response = $this->send( + $this->request('POST', '/api/forgot', [ + 'authenticatedAs' => 2, + 'json' => [ + 'email' => 'normal@machine.local' + ] + ]) + ); + + // Additional Tokens + EmailToken::generate('new-normal@machine.local', 2)->save(); + EmailToken::generate('new-normal@machine.local', 2)->save(); + + $this->assertEquals(204, $response->getStatusCode()); + $this->assertEquals(2, EmailToken::query()->where('user_id', 2)->count()); + + // Use a token to reset password + $response = $this->send( + $request = $this->requestWithCsrfToken( + $this->request('POST', '/reset', [ + 'authenticatedAs' => 2, + ])->withParsedBody([ + 'passwordToken' => PasswordToken::query()->latest()->first()->token, + 'password' => 'new-password', + 'password_confirmation' => 'new-password', + ]) + ) + ); + + $this->assertEquals(302, $response->getStatusCode()); + $this->assertEquals(0, EmailToken::query()->where('user_id', 2)->count()); + } + + /** @test */ + public function password_tokens_are_deleted_when_confirming_email() + { + $this->app(); + + PasswordToken::generate(2)->save(); + PasswordToken::generate(2)->save(); + + $token = EmailToken::generate('new-normal@machine.local', 2); + $token->save(); + + $response = $this->send( + $this->requestWithCsrfToken( + $this->request('POST', '/confirm/'.$token->token, [ + 'authenticatedAs' => 2 + ]) + ) + ); + + $this->assertEquals(302, $response->getStatusCode()); + $this->assertEquals(0, PasswordToken::query()->where('user_id', 2)->count()); + } +} diff --git a/php-packages/testing/src/integration/BuildsHttpRequests.php b/php-packages/testing/src/integration/BuildsHttpRequests.php index 6b379702a..14308eb58 100644 --- a/php-packages/testing/src/integration/BuildsHttpRequests.php +++ b/php-packages/testing/src/integration/BuildsHttpRequests.php @@ -14,6 +14,7 @@ use Dflydev\FigCookies\SetCookie; use Illuminate\Support\Str; use Laminas\Diactoros\CallbackStream; use Psr\Http\Message\ResponseInterface as Response; +use Psr\Http\Message\ServerRequestInterface; use Psr\Http\Message\ServerRequestInterface as Request; /** @@ -66,4 +67,15 @@ trait BuildsHttpRequests return $req->withCookieParams($cookies); } + + protected function requestWithCsrfToken(ServerRequestInterface $request): ServerRequestInterface + { + $initial = $this->send( + $this->request('GET', '/') + ); + + $token = $initial->getHeaderLine('X-CSRF-Token'); + + return $this->requestWithCookiesFrom($request->withHeader('X-CSRF-Token', $token), $initial); + } }