Update and test variable validator logic

app/Services/Servers/VariableValidatorService.php

@@ -11,32 +11,15 @@ namespace Pterodactyl\Services\Servers;
 
 use Pterodactyl\Models\User;
 use Illuminate\Support\Collection;
+use Pterodactyl\Models\EggVariable;
 use Illuminate\Validation\ValidationException;
 use Pterodactyl\Traits\Services\HasUserLevels;
-use Pterodactyl\Contracts\Repository\ServerRepositoryInterface;
 use Illuminate\Contracts\Validation\Factory as ValidationFactory;
-use Pterodactyl\Contracts\Repository\EggVariableRepositoryInterface;
-use Pterodactyl\Contracts\Repository\ServerVariableRepositoryInterface;
 
 class VariableValidatorService
 {
     use HasUserLevels;
 
-    /**
-     * @var \Pterodactyl\Contracts\Repository\EggVariableRepositoryInterface
-     */
-    private $optionVariableRepository;
-
-    /**
-     * @var \Pterodactyl\Contracts\Repository\ServerRepositoryInterface
-     */
-    private $serverRepository;
-
-    /**
-     * @var \Pterodactyl\Contracts\Repository\ServerVariableRepositoryInterface
-     */
-    private $serverVariableRepository;
-
     /**
      * @var \Illuminate\Contracts\Validation\Factory
      */
@@ -45,20 +28,10 @@ class VariableValidatorService
     /**
      * VariableValidatorService constructor.
      *
-     * @param \Pterodactyl\Contracts\Repository\EggVariableRepositoryInterface $optionVariableRepository
-     * @param \Pterodactyl\Contracts\Repository\ServerRepositoryInterface $serverRepository
-     * @param \Pterodactyl\Contracts\Repository\ServerVariableRepositoryInterface $serverVariableRepository
      * @param \Illuminate\Contracts\Validation\Factory $validator
      */
-    public function __construct(
+    public function __construct(ValidationFactory $validator)
-        EggVariableRepositoryInterface $optionVariableRepository,
+    {
-        ServerRepositoryInterface $serverRepository,
-        ServerVariableRepositoryInterface $serverVariableRepository,
-        ValidationFactory $validator
-    ) {
-        $this->optionVariableRepository = $optionVariableRepository;
-        $this->serverRepository = $serverRepository;
-        $this->serverVariableRepository = $serverVariableRepository;
         $this->validator = $validator;
     }
 
@@ -72,16 +45,18 @@ class VariableValidatorService
      */
     public function handle(int $egg, array $fields = []): Collection
     {
-        $variables = $this->optionVariableRepository->findWhere([['egg_id', '=', $egg]]);
+        $query = EggVariable::query()->where('egg_id', $egg);
+        if (! $this->isUserLevel(User::USER_LEVEL_ADMIN)) {
+            // Don't attempt to validate variables if they aren't user editable
+            // and we're not running this at an admin level.
+            $query = $query->where('user_editable', true)->where('user_viewable', true);
+        }
+
+        /** @var \Pterodactyl\Models\EggVariable[] $variables */
+        $variables = $query->get();
 
         $data = $rules = $customAttributes = [];
         foreach ($variables as $variable) {
-            // Don't attempt to validate variables if they aren't user editable
-            // and we're not running this at an admin level.
-            if (! $variable->user_editable && ! $this->isUserLevel(User::USER_LEVEL_ADMIN)) {
-                continue;
-            }
-
             $data['environment'][$variable->env_variable] = array_get($fields, $variable->env_variable);
             $rules['environment.' . $variable->env_variable] = $variable->rules;
             $customAttributes['environment.' . $variable->env_variable] = trans('validation.internal.variable_value', ['env' => $variable->name]);
@@ -92,23 +67,12 @@ class VariableValidatorService
             throw new ValidationException($validator);
         }
 
-        $response = $variables->filter(function ($item) {
+        return Collection::make($variables)->map(function ($item) use ($fields) {
-            // Skip doing anything if user is not an admin and variable is not user viewable or editable.
+            return (object)[
-            if (! $this->isUserLevel(User::USER_LEVEL_ADMIN) && (! $item->user_editable || ! $item->user_viewable)) {
-                return false;
-            }
-
-            return true;
-        })->map(function ($item) use ($fields) {
-            return (object) [
                 'id' => $item->id,
                 'key' => $item->env_variable,
-                'value' => array_get($fields, $item->env_variable),
+                'value' => $fields[$item->env_variable] ?? null,
             ];
-        })->filter(function ($item) {
-            return is_object($item);
         });
-
-        return $response;
     }
 }

tests/Integration/Services/Servers/VariableValidatorServiceTest.php

@@ -0,0 +1,137 @@
+<?php
+
+namespace Pterodactyl\Tests\Integration\Services\Servers;
+
+use Pterodactyl\Models\Egg;
+use Pterodactyl\Models\User;
+use Illuminate\Support\Collection;
+use Illuminate\Validation\ValidationException;
+use Pterodactyl\Tests\Integration\IntegrationTestCase;
+use Pterodactyl\Services\Servers\VariableValidatorService;
+
+class VariableValidatorServiceTest extends IntegrationTestCase
+{
+    /**
+     * Test that enviornment variables for a server are validated as expected.
+     */
+    public function testEnvironmentVariablesCanBeValidated()
+    {
+        /** @noinspection PhpParamsInspection */
+        $egg = $this->cloneEggAndVariables(Egg::query()->findOrFail(1));
+
+        try {
+            $this->getService()->handle($egg->id, [
+                'BUNGEE_VERSION' => '1.2.3',
+            ]);
+
+            $this->assertTrue(false, 'This statement should not be reached.');
+        } catch (ValidationException $exception) {
+            $errors = $exception->errors();
+
+            $this->assertCount(2, $errors);
+            $this->assertArrayHasKey('environment.BUNGEE_VERSION', $errors);
+            $this->assertArrayHasKey('environment.SERVER_JARFILE', $errors);
+            $this->assertSame('The Bungeecord Version variable may only contain letters and numbers.', $errors['environment.BUNGEE_VERSION'][0]);
+            $this->assertSame('The Bungeecord Jar File variable field is required.', $errors['environment.SERVER_JARFILE'][0]);
+        }
+
+        $response = $this->getService()->handle($egg->id, [
+            'BUNGEE_VERSION' => '1234',
+            'SERVER_JARFILE' => 'server.jar',
+        ]);
+
+        $this->assertInstanceOf(Collection::class, $response);
+        $this->assertCount(2, $response);
+        $this->assertSame('BUNGEE_VERSION', $response->get(0)->key);
+        $this->assertSame('1234', $response->get(0)->value);
+        $this->assertSame('SERVER_JARFILE', $response->get(1)->key);
+        $this->assertSame('server.jar', $response->get(1)->value);
+    }
+
+    /**
+     * Test that variables that are user_editable=false do not get validated (or returned) by
+     * the handler.
+     */
+    public function testNormalUserCannotValidateNonUserEditableVariables()
+    {
+        /** @noinspection PhpParamsInspection */
+        $egg = $this->cloneEggAndVariables(Egg::query()->findOrFail(1));
+        $egg->variables()->first()->update([
+            'user_editable' => false,
+        ]);
+
+        $response = $this->getService()->handle($egg->id, [
+            // This is an invalid value, but it shouldn't cause any issues since it should be skipped.
+            'BUNGEE_VERSION' => '1.2.3',
+            'SERVER_JARFILE' => 'server.jar',
+        ]);
+
+        $this->assertInstanceOf(Collection::class, $response);
+        $this->assertCount(1, $response);
+        $this->assertSame('SERVER_JARFILE', $response->get(0)->key);
+        $this->assertSame('server.jar', $response->get(0)->value);
+    }
+
+    public function testEnvironmentVariablesCanBeUpdatedAsAdmin()
+    {
+        /** @noinspection PhpParamsInspection */
+        $egg = $this->cloneEggAndVariables(Egg::query()->findOrFail(1));
+        $egg->variables()->first()->update([
+            'user_editable' => false,
+        ]);
+
+        try {
+            $this->getService()->setUserLevel(User::USER_LEVEL_ADMIN)->handle($egg->id, [
+                'BUNGEE_VERSION' => '1.2.3',
+                'SERVER_JARFILE' => 'server.jar',
+            ]);
+
+            $this->assertTrue(false, 'This statement should not be reached.');
+        } catch (ValidationException $exception) {
+            $this->assertCount(1, $exception->errors());
+            $this->assertArrayHasKey('environment.BUNGEE_VERSION', $exception->errors());
+        }
+
+
+        $response = $this->getService()->setUserLevel(User::USER_LEVEL_ADMIN)->handle($egg->id, [
+            'BUNGEE_VERSION' => '123',
+            'SERVER_JARFILE' => 'server.jar',
+        ]);
+
+        $this->assertInstanceOf(Collection::class, $response);
+        $this->assertCount(2, $response);
+        $this->assertSame('BUNGEE_VERSION', $response->get(0)->key);
+        $this->assertSame('123', $response->get(0)->value);
+        $this->assertSame('SERVER_JARFILE', $response->get(1)->key);
+        $this->assertSame('server.jar', $response->get(1)->value);
+    }
+
+    public function testNullableEnvironmentVariablesCanBeUsedCorrectly()
+    {
+        /** @noinspection PhpParamsInspection */
+        $egg = $this->cloneEggAndVariables(Egg::query()->findOrFail(1));
+        $egg->variables()->where('env_variable', '!=', 'BUNGEE_VERSION')->delete();
+
+        $egg->variables()->update(['rules' => 'nullable|string']);
+
+        $response = $this->getService()->handle($egg->id, []);
+        $this->assertCount(1, $response);
+        $this->assertNull($response->get(0)->value);
+
+        $response = $this->getService()->handle($egg->id, ['BUNGEE_VERSION' => null]);
+        $this->assertCount(1, $response);
+        $this->assertNull($response->get(0)->value);
+
+        $response = $this->getService()->handle($egg->id, ['BUNGEE_VERSION' => '']);
+        $this->assertCount(1, $response);
+        $this->assertSame('', $response->get(0)->value);
+    }
+
+    /**
+     * @return \Pterodactyl\Services\Servers\VariableValidatorService
+     */
+    private function getService()
+    {
+        return $this->app->make(VariableValidatorService::class);
+    }
+}

tests/Unit/Services/Servers/VariableValidatorServiceTest.php

@@ -1,175 +0,0 @@
-<?php
-
-namespace Tests\Unit\Services\Servers;
-
-use Mockery as m;
-use Tests\TestCase;
-use Pterodactyl\Models\User;
-use Illuminate\Support\Collection;
-use Pterodactyl\Models\EggVariable;
-use Illuminate\Contracts\Validation\Factory;
-use Illuminate\Validation\ValidationException;
-use Pterodactyl\Services\Servers\VariableValidatorService;
-use Pterodactyl\Contracts\Repository\ServerRepositoryInterface;
-use Pterodactyl\Contracts\Repository\EggVariableRepositoryInterface;
-use Pterodactyl\Contracts\Repository\ServerVariableRepositoryInterface;
-
-class VariableValidatorServiceTest extends TestCase
-{
-    /**
-     * @var \Pterodactyl\Contracts\Repository\EggVariableRepositoryInterface|\Mockery\Mock
-     */
-    private $optionVariableRepository;
-
-    /**
-     * @var \Pterodactyl\Contracts\Repository\ServerRepositoryInterface|\Mockery\Mock
-     */
-    private $serverRepository;
-
-    /**
-     * @var \Pterodactyl\Contracts\Repository\ServerVariableRepositoryInterface|\Mockery\Mock
-     */
-    private $serverVariableRepository;
-
-    /**
-     * Setup tests.
-     */
-    public function setUp(): void
-    {
-        parent::setUp();
-
-        $this->optionVariableRepository = m::mock(EggVariableRepositoryInterface::class);
-        $this->serverRepository = m::mock(ServerRepositoryInterface::class);
-        $this->serverVariableRepository = m::mock(ServerVariableRepositoryInterface::class);
-    }
-
-    /**
-     * Test that when no variables are found for an option no data is returned.
-     */
-    public function testEmptyResultSetShouldBeReturnedIfNoVariablesAreFound()
-    {
-        $this->optionVariableRepository->shouldReceive('findWhere')->with([['egg_id', '=', 1]])->andReturn(collect([]));
-
-        $response = $this->getService()->handle(1, []);
-        $this->assertEmpty($response);
-        $this->assertInstanceOf(Collection::class, $response);
-    }
-
-    /**
-     * Test that variables set as user_editable=0 and/or user_viewable=0 are skipped when admin flag is not set.
-     */
-    public function testValidatorShouldNotProcessVariablesSetAsNotUserEditableWhenAdminFlagIsNotPassed()
-    {
-        $variables = $this->getVariableCollection();
-        $this->optionVariableRepository->shouldReceive('findWhere')->with([['egg_id', '=', 1]])->andReturn($variables);
-
-        $response = $this->getService()->handle(1, [
-            $variables[0]->env_variable => 'Test_SomeValue_0',
-            $variables[1]->env_variable => 'Test_SomeValue_1',
-            $variables[2]->env_variable => 'Test_SomeValue_2',
-            $variables[3]->env_variable => 'Test_SomeValue_3',
-        ]);
-
-        $this->assertNotEmpty($response);
-        $this->assertInstanceOf(Collection::class, $response);
-        $this->assertEquals(1, $response->count(), 'Assert response has a single item in collection.');
-
-        $variable = $response->first();
-        $this->assertObjectHasAttribute('id', $variable);
-        $this->assertObjectHasAttribute('key', $variable);
-        $this->assertObjectHasAttribute('value', $variable);
-        $this->assertSame($variables[0]->id, $variable->id);
-        $this->assertSame($variables[0]->env_variable, $variable->key);
-        $this->assertSame('Test_SomeValue_0', $variable->value);
-    }
-
-    /**
-     * Test that all variables are processed correctly if admin flag is set.
-     */
-    public function testValidatorShouldProcessAllVariablesWhenAdminFlagIsSet()
-    {
-        $variables = $this->getVariableCollection();
-        $this->optionVariableRepository->shouldReceive('findWhere')->with([['egg_id', '=', 1]])->andReturn($variables);
-
-        $service = $this->getService();
-        $service->setUserLevel(User::USER_LEVEL_ADMIN);
-        $response = $service->handle(1, [
-            $variables[0]->env_variable => 'Test_SomeValue_0',
-            $variables[1]->env_variable => 'Test_SomeValue_1',
-            $variables[2]->env_variable => 'Test_SomeValue_2',
-            $variables[3]->env_variable => 'Test_SomeValue_3',
-        ]);
-
-        $this->assertNotEmpty($response);
-        $this->assertInstanceOf(Collection::class, $response);
-        $this->assertEquals(4, $response->count(), 'Assert response has all four items in collection.');
-
-        $response->each(function ($variable, $key) use ($variables) {
-            $this->assertObjectHasAttribute('id', $variable);
-            $this->assertObjectHasAttribute('key', $variable);
-            $this->assertObjectHasAttribute('value', $variable);
-            $this->assertSame($variables[$key]->id, $variable->id);
-            $this->assertSame($variables[$key]->env_variable, $variable->key);
-            $this->assertSame('Test_SomeValue_' . $key, $variable->value);
-        });
-    }
-
-    /**
-     * Test that a DisplayValidationError is thrown when a variable is not validated.
-     */
-    public function testValidatorShouldThrowExceptionWhenAValidationErrorIsEncountered()
-    {
-        $variables = $this->getVariableCollection();
-        $this->optionVariableRepository->shouldReceive('findWhere')->with([['egg_id', '=', 1]])->andReturn($variables);
-
-        try {
-            $this->getService()->handle(1, [$variables[0]->env_variable => null]);
-        } catch (ValidationException $exception) {
-            $messages = $exception->validator->getMessageBag()->all();
-
-            $this->assertNotEmpty($messages);
-            $this->assertSame(2, count($messages));
-
-            // We only expect to get the first two variables form the getVariableCollection
-            // function here since those are the only two that are editable, and the others
-            // should be discarded and not validated.
-            for ($i = 0; $i < 2; $i++) {
-                $this->assertSame(trans('validation.required', [
-                    'attribute' => trans('validation.internal.variable_value', ['env' => $variables[$i]->name]),
-                ]), $messages[$i]);
-            }
-        }
-    }
-
-    /**
-     * Return a collection of fake variables to use for testing.
-     *
-     * @return \Illuminate\Support\Collection
-     */
-    private function getVariableCollection(): Collection
-    {
-        return collect(
-            [
-                factory(EggVariable::class)->states('editable', 'viewable')->make(),
-                factory(EggVariable::class)->states('editable')->make(),
-                factory(EggVariable::class)->states('viewable')->make(),
-                factory(EggVariable::class)->make(),
-            ]
-        );
-    }
-
-    /**
-     * Return an instance of the service with mocked dependencies.
-     *
-     * @return \Pterodactyl\Services\Servers\VariableValidatorService
-     */
-    private function getService(): VariableValidatorService
-    {
-        return new VariableValidatorService(
-            $this->optionVariableRepository,
-            $this->serverRepository,
-            $this->serverVariableRepository,
-            $this->app->make(Factory::class)
-        );
-    }
-}