diff --git a/CHANGELOG.md b/CHANGELOG.md index 80d6d9065..848278c46 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ This project follows [Semantic Versioning](http://semver.org) guidelines. * `[rc.2]` — Fixes Admin CP user editing resetting a password on users unintentionally. * `[rc.2]` — Fixes bug with server creation API endpoint that would fail to validate `allocation.default` correctly. * `[rc.2]` — Fix data integrity exception occuring due to invalid data being passed to server creation service on the API. +* `[rc.2]` — Fix data integrity exception that could occur when an email containing non-username characters was passed. ### Added * Added ability to search the following API endpoints: list users, list servers, and list locations. diff --git a/app/Http/Controllers/API/Remote/SftpController.php b/app/Http/Controllers/API/Remote/SftpController.php index 2e12d4e51..083544237 100644 --- a/app/Http/Controllers/API/Remote/SftpController.php +++ b/app/Http/Controllers/API/Remote/SftpController.php @@ -42,7 +42,12 @@ class SftpController extends Controller */ public function index(SftpAuthenticationFormRequest $request): JsonResponse { - $connection = explode('.', $request->input('username')); + $parts = explode('.', strrev($request->input('username')), 2); + $connection = [ + 'username' => strrev(array_get($parts, 1)), + 'server' => strrev(array_get($parts, 0)), + ]; + $this->incrementLoginAttempts($request); if ($this->hasTooManyLoginAttempts($request)) { @@ -53,10 +58,10 @@ class SftpController extends Controller try { $data = $this->authenticationService->handle( - array_get($connection, 0), + $connection['username'], $request->input('password'), object_get($request->attributes->get('node'), 'id', 0), - array_get($connection, 1) + empty($connection['server']) ? null : $connection['server'] ); $this->clearLoginAttempts($request); diff --git a/app/Models/User.php b/app/Models/User.php index 26109cc0b..88960994f 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -4,6 +4,7 @@ namespace Pterodactyl\Models; use Sofa\Eloquence\Eloquence; use Sofa\Eloquence\Validable; +use Pterodactyl\Rules\Username; use Illuminate\Validation\Rules\In; use Illuminate\Auth\Authenticatable; use Illuminate\Database\Eloquent\Model; @@ -151,7 +152,7 @@ class User extends Model implements 'uuid' => 'string|size:36|unique:users,uuid', 'email' => 'email|unique:users,email', 'external_id' => 'nullable|string|max:255|unique:users,external_id', - 'username' => 'alpha_dash|between:1,255|unique:users,username', + 'username' => 'between:1,255|unique:users,username', 'name_first' => 'string|between:1,255', 'name_last' => 'string|between:1,255', 'password' => 'nullable|string', @@ -169,6 +170,7 @@ class User extends Model implements { $rules = self::eloquenceGatherRules(); $rules['language'][] = new In(array_keys((new self)->getAvailableLanguages())); + $rules['username'][] = new Username; return $rules; } @@ -188,9 +190,9 @@ class User extends Model implements * * @param string $value */ - public function setUsernameAttribute($value) + public function setUsernameAttribute(string $value) { - $this->attributes['username'] = strtolower($value); + $this->attributes['username'] = mb_strtolower($value); } /** diff --git a/app/Rules/Username.php b/app/Rules/Username.php new file mode 100644 index 000000000..08d9baba0 --- /dev/null +++ b/app/Rules/Username.php @@ -0,0 +1,36 @@ +userCreationService->handle([ 'email' => $email, - 'username' => substr(strtok($email, '@'), 0, 8) . '_' . str_random(6), + 'username' => $username . str_random(3), 'name_first' => 'Server', 'name_last' => 'Subuser', 'root_admin' => false, diff --git a/tests/Unit/Rules/UsernameTest.php b/tests/Unit/Rules/UsernameTest.php new file mode 100644 index 000000000..fa21530af --- /dev/null +++ b/tests/Unit/Rules/UsernameTest.php @@ -0,0 +1,69 @@ +assertTrue((new Username)->passes('test', $username), 'Assert username is valid.'); + } + + /** + * Test invalid usernames return false. + * + * @dataProvider invalidUsernameDataProvider + */ + public function testInvalidUsernames(string $username) + { + $this->assertFalse((new Username)->passes('test', $username), 'Assert username is not valid.'); + } + + /** + * Provide valid usernames. + * @return array + */ + public function validUsernameDataProvider(): array + { + return [ + ['username'], + ['user_name'], + ['user.name'], + ['user-name'], + ['123username123'], + ['123-user.name'], + ['123456'], + ]; + } + + /** + * Provide invalid usernames. + * + * @return array + */ + public function invalidUsernameDataProvider(): array + { + return [ + ['_username'], + ['username_'], + ['_username_'], + ['-username'], + ['.username'], + ['username-'], + ['username.'], + ['user*name'], + ['user^name'], + ['user#name'], + ['user+name'], + ['1234_'], + ]; + } +} diff --git a/tests/Unit/Services/Subusers/SubuserCreationServiceTest.php b/tests/Unit/Services/Subusers/SubuserCreationServiceTest.php index 93d976bf6..97b68ffc7 100644 --- a/tests/Unit/Services/Subusers/SubuserCreationServiceTest.php +++ b/tests/Unit/Services/Subusers/SubuserCreationServiceTest.php @@ -1,17 +1,9 @@ . - * - * This software is licensed under the terms of the MIT license. - * https://opensource.org/licenses/MIT - */ namespace Tests\Unit\Services\Subusers; use Mockery as m; use Tests\TestCase; -use phpmock\phpunit\PHPMock; use Pterodactyl\Models\User; use Pterodactyl\Models\Server; use Pterodactyl\Models\Subuser; @@ -30,8 +22,6 @@ use Pterodactyl\Exceptions\Service\Subuser\ServerSubuserExistsException; class SubuserCreationServiceTest extends TestCase { - use PHPMock; - /** * @var \Illuminate\Database\ConnectionInterface|\Mockery\Mock */ @@ -79,8 +69,6 @@ class SubuserCreationServiceTest extends TestCase { parent::setUp(); - $this->getFunctionMock('\\Pterodactyl\\Services\\Subusers', 'str_random')->expects($this->any())->willReturn('random_string'); - $this->connection = m::mock(ConnectionInterface::class); $this->keyCreationService = m::mock(DaemonKeyCreationService::class); $this->permissionService = m::mock(PermissionCreationService::class); @@ -88,16 +76,6 @@ class SubuserCreationServiceTest extends TestCase $this->serverRepository = m::mock(ServerRepositoryInterface::class); $this->userCreationService = m::mock(UserCreationService::class); $this->userRepository = m::mock(UserRepositoryInterface::class); - - $this->service = new SubuserCreationService( - $this->connection, - $this->keyCreationService, - $this->permissionService, - $this->serverRepository, - $this->subuserRepository, - $this->userCreationService, - $this->userRepository - ); } /** @@ -107,18 +85,25 @@ class SubuserCreationServiceTest extends TestCase { $permissions = ['test-1' => 'test:1', 'test-2' => null]; $server = factory(Server::class)->make(); - $user = factory(User::class)->make(); + $user = factory(User::class)->make([ + 'email' => 'known.1+test@example.com', + ]); $subuser = factory(Subuser::class)->make(['user_id' => $user->id, 'server_id' => $server->id]); $this->connection->shouldReceive('beginTransaction')->withNoArgs()->once()->andReturnNull(); $this->userRepository->shouldReceive('findFirstWhere')->with([['email', '=', $user->email]])->once()->andThrow(new RecordNotFoundException); - $this->userCreationService->shouldReceive('handle')->with([ - 'email' => $user->email, - 'username' => substr(strtok($user->email, '@'), 0, 8) . '_' . 'random_string', - 'name_first' => 'Server', - 'name_last' => 'Subuser', - 'root_admin' => false, - ])->once()->andReturn($user); + $this->userCreationService->shouldReceive('handle')->with(m::on(function ($data) use ($user) { + $subset = m::subset([ + 'email' => $user->email, + 'name_first' => 'Server', + 'name_last' => 'Subuser', + 'root_admin' => false, + ])->match($data); + + $username = substr(array_get($data, 'username', ''), 0, -3) === 'known.1test'; + + return $subset && $username; + }))->once()->andReturn($user); $this->subuserRepository->shouldReceive('create')->with(['user_id' => $user->id, 'server_id' => $server->id]) ->once()->andReturn($subuser); @@ -126,7 +111,7 @@ class SubuserCreationServiceTest extends TestCase $this->permissionService->shouldReceive('handle')->with($subuser->id, array_keys($permissions))->once()->andReturnNull(); $this->connection->shouldReceive('commit')->withNoArgs()->once()->andReturnNull(); - $response = $this->service->handle($server, $user->email, array_keys($permissions)); + $response = $this->getService()->handle($server, $user->email, array_keys($permissions)); $this->assertInstanceOf(Subuser::class, $response); $this->assertSame($subuser, $response); } @@ -155,7 +140,7 @@ class SubuserCreationServiceTest extends TestCase $this->permissionService->shouldReceive('handle')->with($subuser->id, $permissions)->once()->andReturnNull(); $this->connection->shouldReceive('commit')->withNoArgs()->once()->andReturnNull(); - $response = $this->service->handle($server->id, $user->email, $permissions); + $response = $this->getService()->handle($server->id, $user->email, $permissions); $this->assertInstanceOf(Subuser::class, $response); $this->assertSame($subuser, $response); } @@ -172,7 +157,7 @@ class SubuserCreationServiceTest extends TestCase $this->userRepository->shouldReceive('findFirstWhere')->with([['email', '=', $user->email]])->once()->andReturn($user); try { - $this->service->handle($server, $user->email, []); + $this->getService()->handle($server, $user->email, []); } catch (DisplayException $exception) { $this->assertInstanceOf(UserIsServerOwnerException::class, $exception); $this->assertEquals(trans('exceptions.subusers.user_is_owner'), $exception->getMessage()); @@ -195,10 +180,28 @@ class SubuserCreationServiceTest extends TestCase ])->once()->andReturn(1); try { - $this->service->handle($server, $user->email, []); + $this->getService()->handle($server, $user->email, []); } catch (DisplayException $exception) { $this->assertInstanceOf(ServerSubuserExistsException::class, $exception); $this->assertEquals(trans('exceptions.subusers.subuser_exists'), $exception->getMessage()); } } + + /** + * Return an instance of the service with mocked dependencies. + * + * @return \Pterodactyl\Services\Subusers\SubuserCreationService + */ + private function getService(): SubuserCreationService + { + return new SubuserCreationService( + $this->connection, + $this->keyCreationService, + $this->permissionService, + $this->serverRepository, + $this->subuserRepository, + $this->userCreationService, + $this->userRepository + ); + } }