From 2b3303c46b99168356e07090797afd0321cabd80 Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Sun, 15 Aug 2021 17:37:12 -0700 Subject: [PATCH] Fix changing a user password to not incorrectly handle logging out old sessions; closes #3531 --- .../Api/Client/AccountController.php | 11 ++++++--- .../Api/Client/Account/UpdateEmailRequest.php | 6 ++++- .../Client/Account/UpdatePasswordRequest.php | 6 ++++- app/Services/Users/UserUpdateService.php | 23 +++++-------------- .../Api/Client/AccountControllerTest.php | 14 ++++++----- 5 files changed, 32 insertions(+), 28 deletions(-) diff --git a/app/Http/Controllers/Api/Client/AccountController.php b/app/Http/Controllers/Api/Client/AccountController.php index 5e790d474..ead296d3c 100644 --- a/app/Http/Controllers/Api/Client/AccountController.php +++ b/app/Http/Controllers/Api/Client/AccountController.php @@ -58,12 +58,17 @@ class AccountController extends ClientApiController * Update the authenticated user's password. All existing sessions will be logged * out immediately. * - * @throws \Pterodactyl\Exceptions\Model\DataValidationException - * @throws \Pterodactyl\Exceptions\Repository\RecordNotFoundException + * @throws \Throwable */ public function updatePassword(UpdatePasswordRequest $request): JsonResponse { - $this->updateService->handle($request->user(), $request->validated()); + $user = $this->updateService->handle($request->user(), $request->validated()); + + // If you do not update the user in the session you'll end up working with a + // cached copy of the user that does not include the updated password. Do this + // to correctly store the new user details in the guard and allow the logout + // other devices functionality to work. + $this->sessionGuard->setUser($user); $this->sessionGuard->logoutOtherDevices($request->input('password')); diff --git a/app/Http/Requests/Api/Client/Account/UpdateEmailRequest.php b/app/Http/Requests/Api/Client/Account/UpdateEmailRequest.php index 193d2fd70..6287ba585 100644 --- a/app/Http/Requests/Api/Client/Account/UpdateEmailRequest.php +++ b/app/Http/Requests/Api/Client/Account/UpdateEmailRequest.php @@ -3,6 +3,8 @@ namespace Pterodactyl\Http\Requests\Api\Client\Account; use Pterodactyl\Models\User; +use Illuminate\Container\Container; +use Illuminate\Contracts\Hashing\Hasher; use Pterodactyl\Http\Requests\Api\Client\ClientApiRequest; use Pterodactyl\Exceptions\Http\Base\InvalidPasswordProvidedException; @@ -17,8 +19,10 @@ class UpdateEmailRequest extends ClientApiRequest return false; } + $hasher = Container::getInstance()->make(Hasher::class); + // Verify password matches when changing password or email. - if (!password_verify($this->input('password'), $this->user()->password)) { + if (!$hasher->check($this->input('password'), $this->user()->password)) { throw new InvalidPasswordProvidedException(trans('validation.internal.invalid_password')); } diff --git a/app/Http/Requests/Api/Client/Account/UpdatePasswordRequest.php b/app/Http/Requests/Api/Client/Account/UpdatePasswordRequest.php index c0e27bdb5..de8215b07 100644 --- a/app/Http/Requests/Api/Client/Account/UpdatePasswordRequest.php +++ b/app/Http/Requests/Api/Client/Account/UpdatePasswordRequest.php @@ -2,6 +2,8 @@ namespace Pterodactyl\Http\Requests\Api\Client\Account; +use Illuminate\Container\Container; +use Illuminate\Contracts\Hashing\Hasher; use Pterodactyl\Http\Requests\Api\Client\ClientApiRequest; use Pterodactyl\Exceptions\Http\Base\InvalidPasswordProvidedException; @@ -16,8 +18,10 @@ class UpdatePasswordRequest extends ClientApiRequest return false; } + $hasher = Container::getInstance()->make(Hasher::class); + // Verify password matches when changing password or email. - if (!password_verify($this->input('current_password'), $this->user()->password)) { + if (!$hasher->check($this->input('current_password'), $this->user()->password)) { throw new InvalidPasswordProvidedException(trans('validation.internal.invalid_password')); } diff --git a/app/Services/Users/UserUpdateService.php b/app/Services/Users/UserUpdateService.php index 0c7e1c172..31f4010b6 100644 --- a/app/Services/Users/UserUpdateService.php +++ b/app/Services/Users/UserUpdateService.php @@ -5,7 +5,6 @@ namespace Pterodactyl\Services\Users; use Pterodactyl\Models\User; use Illuminate\Contracts\Hashing\Hasher; use Pterodactyl\Traits\Services\HasUserLevels; -use Pterodactyl\Repositories\Eloquent\UserRepository; class UserUpdateService { @@ -16,29 +15,20 @@ class UserUpdateService */ private $hasher; - /** - * @var \Pterodactyl\Repositories\Eloquent\UserRepository - */ - private $repository; - /** * UpdateService constructor. */ - public function __construct(Hasher $hasher, UserRepository $repository) + public function __construct(Hasher $hasher) { $this->hasher = $hasher; - $this->repository = $repository; } /** - * Update the user model instance. + * Update the user model instance and return the updated model. * - * @return \Pterodactyl\Models\User - * - * @throws \Pterodactyl\Exceptions\Model\DataValidationException - * @throws \Pterodactyl\Exceptions\Repository\RecordNotFoundException + * @throws \Throwable */ - public function handle(User $user, array $data) + public function handle(User $user, array $data): User { if (!empty(array_get($data, 'password'))) { $data['password'] = $this->hasher->make($data['password']); @@ -46,9 +36,8 @@ class UserUpdateService unset($data['password']); } - /** @var \Pterodactyl\Models\User $response */ - $response = $this->repository->update($user->id, $data); + $user->forceFill($data)->saveOrFail(); - return $response; + return $user->refresh(); } } diff --git a/tests/Integration/Api/Client/AccountControllerTest.php b/tests/Integration/Api/Client/AccountControllerTest.php index 3ed689ef9..971b83421 100644 --- a/tests/Integration/Api/Client/AccountControllerTest.php +++ b/tests/Integration/Api/Client/AccountControllerTest.php @@ -2,10 +2,9 @@ namespace Pterodactyl\Tests\Integration\Api\Client; -use Mockery; use Pterodactyl\Models\User; use Illuminate\Http\Response; -use Illuminate\Auth\AuthManager; +use Illuminate\Support\Facades\Hash; class AccountControllerTest extends ClientApiIntegrationTestCase { @@ -106,10 +105,7 @@ class AccountControllerTest extends ClientApiIntegrationTestCase /** @var \Pterodactyl\Models\User $user */ $user = User::factory()->create(); - $mock = Mockery::mock(AuthManager::class); - $mock->expects('logoutOtherDevices')->with('New_Password1'); - - $this->app->instance(AuthManager::class, $mock); + $initialHash = $user->password; $response = $this->actingAs($user)->putJson('/api/client/account/password', [ 'current_password' => 'password', @@ -117,6 +113,12 @@ class AccountControllerTest extends ClientApiIntegrationTestCase 'password_confirmation' => 'New_Password1', ]); + $user = $user->refresh(); + + $this->assertNotEquals($user->password, $initialHash); + $this->assertTrue(Hash::check('New_Password1', $user->password)); + $this->assertFalse(Hash::check('password', $user->password)); + $response->assertStatus(Response::HTTP_NO_CONTENT); }