Fix username validation and auto-generation, closes pterodactyl#927

DaneEveritt · DaneEveritt · commit bf537922a346 · 2018-02-11T16:39:50.000-06:00
diff --git 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
@@ -42,7 +42,12 @@ public function __construct(AuthenticateUsingPasswordService $authenticationServ
      */
     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 @@ public function index(SftpAuthenticationFormRequest $request): JsonResponse
 
         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
@@ -4,6 +4,7 @@
 
 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 @@ protected static function gatherRules()
     {
         $rules = self::eloquenceGatherRules();
         $rules['language'][] = new In(array_keys((new self)->getAvailableLanguages()));
+        $rules['username'][] = new Username;
 
         return $rules;
     }
@@ -188,9 +190,9 @@ public function sendPasswordResetNotification($token)
      *
      * @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
@@ -0,0 +1,36 @@
+<?php
+
+namespace Pterodactyl\Rules;
+
+use Illuminate\Contracts\Validation\Rule;
+
+class Username implements Rule
+{
+    public const VALIDATION_REGEX = '/^[a-z0-9]([\w\.-]+)[a-z0-9]$/';
+
+    /**
+     * Validate that a username contains only the allowed characters and starts/ends
+     * with alpha-numeric characters.
+     *
+     * Allowed characters: a-z0-9_-.
+     *
+     * @param string $attribute
+     * @param mixed  $value
+     * @return bool
+     */
+    public function passes($attribute, $value): bool
+    {
+        return preg_match(self::VALIDATION_REGEX, mb_strtolower($value));
+    }
+
+    /**
+     * Return a validation message for use when this rule fails.
+     *
+     * @return string
+     */
+    public function message(): string
+    {
+        return 'The :attribute must start and end with alpha-numeric characters and
+                contain only letters, numbers, dashes, underscores, and periods.';
+    }
+}
diff --git a/app/Services/Subusers/SubuserCreationService.php b/app/Services/Subusers/SubuserCreationService.php
@@ -10,6 +10,7 @@
 namespace Pterodactyl\Services\Subusers;
 
 use Pterodactyl\Models\Server;
+use Pterodactyl\Rules\Username;
 use Illuminate\Database\ConnectionInterface;
 use Pterodactyl\Services\Users\UserCreationService;
 use Pterodactyl\Contracts\Repository\UserRepositoryInterface;
@@ -117,9 +118,10 @@ public function handle($server, $email, array $permissions)
                 throw new ServerSubuserExistsException(trans('exceptions.subusers.subuser_exists'));
             }
         } catch (RecordNotFoundException $exception) {
+            $username = preg_replace('/([^\w\.-]+)/', '', strtok($email, '@'));
             $user = $this->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
@@ -0,0 +1,69 @@
+<?php
+
+namespace Tests\Unit\Rules;
+
+use Tests\TestCase;
+use Pterodactyl\Rules\Username;
+
+class UsernameTest extends TestCase
+{
+    /**
+     * Test valid usernames.
+     *
+     * @dataProvider validUsernameDataProvider
+     */
+    public function testValidUsernames(string $username)
+    {
+        $this->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
@@ -1,17 +1,9 @@
 <?php
-/**
- * Pterodactyl - Panel
- * Copyright (c) 2015 - 2017 Dane Everitt <dane@daneeveritt.com>.
- *
- * 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 @@
 
 class SubuserCreationServiceTest extends TestCase
 {
-    use PHPMock;
-
     /**
      * @var \Illuminate\Database\ConnectionInterface|\Mockery\Mock
      */
@@ -79,25 +69,13 @@ public function setUp()
     {
         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);
         $this->subuserRepository = m::mock(SubuserRepositoryInterface::class);
         $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,26 +85,33 @@ public function testAccountIsCreatedForNewUser()
     {
         $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);
         $this->keyCreationService->shouldReceive('handle')->with($server->id, $user->id)->once()->andReturnNull();
         $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 @@ public function testExistingUserCanBeAddedAsASubuser()
         $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 @@ public function testExceptionIsThrownIfUserIsServerOwner()
         $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 @@ public function testExceptionIsThrownIfUserIsAlreadyASubuser()
         ])->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
+        );
+    }
 }
