Added validation to variable validation rules to validate that the validation rules are valid

closes #988

CHANGELOG.md

@@ -9,6 +9,7 @@ This project follows [Semantic Versioning](http://semver.org) guidelines.
 
 ### Added
 * Added a new client API endpoint for gathering the utilization stats for servers including disk, cpu, and memory. `GET /api/client/servers/<id>/utilization`
+* Added validation to variable validation rules to validate that the validation rules are valid because we heard you like validating your validation.
 
 ## v0.7.6 (Derelict Dermodactylus)
 ### Fixed

app/Exceptions/Service/Egg/Variable/BadValidationRuleException.php

@@ -0,0 +1,9 @@
+<?php
+
+namespace Pterodactyl\Exceptions\Service\Egg\Variable;
+
+use Pterodactyl\Exceptions\DisplayException;
+
+class BadValidationRuleException extends DisplayException
+{
+}

app/Services/Eggs/Variables/VariableCreationService.php

@@ -3,24 +3,46 @@
 namespace Pterodactyl\Services\Eggs\Variables;
 
 use Pterodactyl\Models\EggVariable;
+use Illuminate\Contracts\Validation\Factory;
+use Pterodactyl\Traits\Services\ValidatesValidationRules;
 use Pterodactyl\Contracts\Repository\EggVariableRepositoryInterface;
 use Pterodactyl\Exceptions\Service\Egg\Variable\ReservedVariableNameException;
 
 class VariableCreationService
 {
+    use ValidatesValidationRules;
+
     /**
      * @var \Pterodactyl\Contracts\Repository\EggVariableRepositoryInterface
      */
-    protected $repository;
+    private $repository;
+
+    /**
+     * @var \Illuminate\Contracts\Validation\Factory
+     */
+    private $validator;
 
     /**
      * VariableCreationService constructor.
      *
      * @param \Pterodactyl\Contracts\Repository\EggVariableRepositoryInterface $repository
+     * @param \Illuminate\Contracts\Validation\Factory                         $validator
      */
-    public function __construct(EggVariableRepositoryInterface $repository)
+    public function __construct(EggVariableRepositoryInterface $repository, Factory $validator)
     {
         $this->repository = $repository;
+        $this->validator = $validator;
+    }
+
+    /**
+     * Return the validation factory instance to be used by rule validation
+     * checking in the trait.
+     *
+     * @return \Illuminate\Contracts\Validation\Factory
+     */
+    protected function getValidator(): Factory
+    {
+        return $this->validator;
     }
 
     /**
@@ -31,6 +53,7 @@ class VariableCreationService
      * @return \Pterodactyl\Models\EggVariable
      *
      * @throws \Pterodactyl\Exceptions\Model\DataValidationException
+     * @throws \Pterodactyl\Exceptions\Service\Egg\Variable\BadValidationRuleException
      * @throws \Pterodactyl\Exceptions\Service\Egg\Variable\ReservedVariableNameException
      */
     public function handle(int $egg, array $data): EggVariable
@@ -42,6 +65,10 @@ class VariableCreationService
             ));
         }
 
+        if (! empty($data['rules'] ?? '')) {
+            $this->validateRules($data['rules']);
+        }
+
         $options = array_get($data, 'options') ?? [];
 
         return $this->repository->create([

app/Services/Eggs/Variables/VariableUpdateService.php

@@ -3,25 +3,47 @@
 namespace Pterodactyl\Services\Eggs\Variables;
 
 use Pterodactyl\Models\EggVariable;
+use Illuminate\Contracts\Validation\Factory;
 use Pterodactyl\Exceptions\DisplayException;
+use Pterodactyl\Traits\Services\ValidatesValidationRules;
 use Pterodactyl\Contracts\Repository\EggVariableRepositoryInterface;
 use Pterodactyl\Exceptions\Service\Egg\Variable\ReservedVariableNameException;
 
 class VariableUpdateService
 {
+    use ValidatesValidationRules;
+
     /**
      * @var \Pterodactyl\Contracts\Repository\EggVariableRepositoryInterface
      */
-    protected $repository;
+    private $repository;
+
+    /**
+     * @var \Illuminate\Contracts\Validation\Factory
+     */
+    private $validator;
 
     /**
      * VariableUpdateService constructor.
      *
      * @param \Pterodactyl\Contracts\Repository\EggVariableRepositoryInterface $repository
+     * @param \Illuminate\Contracts\Validation\Factory                         $validator
      */
-    public function __construct(EggVariableRepositoryInterface $repository)
+    public function __construct(EggVariableRepositoryInterface $repository, Factory $validator)
     {
         $this->repository = $repository;
+        $this->validator = $validator;
+    }
+
+    /**
+     * Return the validation factory instance to be used by rule validation
+     * checking in the trait.
+     *
+     * @return \Illuminate\Contracts\Validation\Factory
+     */
+    protected function getValidator(): Factory
+    {
+        return $this->validator;
     }
 
     /**
@@ -58,6 +80,10 @@ class VariableUpdateService
             }
         }
 
+        if (! empty($data['rules'] ?? '')) {
+            $this->validateRules($data['rules']);
+        }
+
         $options = array_get($data, 'options') ?? [];
 
         return $this->repository->withoutFreshModel()->update($variable->id, [

app/Traits/Services/ValidatesValidationRules.php

@@ -0,0 +1,40 @@
+<?php
+
+namespace Pterodactyl\Traits\Services;
+
+use BadMethodCallException;
+use Illuminate\Support\Str;
+use Illuminate\Contracts\Validation\Factory;
+use Pterodactyl\Exceptions\Service\Egg\Variable\BadValidationRuleException;
+
+trait ValidatesValidationRules
+{
+    /**
+     * @return \Illuminate\Contracts\Validation\Factory
+     */
+    abstract protected function getValidator(): Factory;
+
+    /**
+     * Validate that the rules being provided are valid for Laravel and can
+     * be resolved.
+     *
+     * @param array|string $rules
+     *
+     * @throws \Pterodactyl\Exceptions\Service\Egg\Variable\BadValidationRuleException
+     */
+    public function validateRules($rules)
+    {
+        try {
+            $this->getValidator()->make(['__TEST' => 'test'], ['__TEST' => $rules])->fails();
+        } catch (BadMethodCallException $exception) {
+            $matches = [];
+            if (preg_match('/Method \[(.+)\] does not exist\./', $exception->getMessage(), $matches)) {
+                throw new BadValidationRuleException(trans('exceptions.nest.variables.bad_validation_rule', [
+                    'rule' => Str::snake(str_replace('validate', '', array_get($matches, 1, 'unknownRule'))),
+                ]), $exception);
+            }
+
+            throw $exception;
+        }
+    }
+}

resources/lang/en/exceptions.php

@@ -24,6 +24,7 @@ return [
         'variables' => [
             'env_not_unique' => 'The environment variable :name must be unique to this Egg.',
             'reserved_name' => 'The environment variable :name is protected and cannot be assigned to a variable.',
+            'bad_validation_rule' => 'The validation rule ":rule" is not a valid rule for this application.',
         ],
         'importer' => [
             'json_error' => 'There was an error while attempting to parse the JSON file: :error.',

resources/themes/pterodactyl/admin/eggs/variables.blade.php

@@ -105,20 +105,20 @@
                 <div class="modal-body">
                     <div class="form-group">
                         <label class="control-label">Name <span class="field-required"></span></label>
-                        <input type="text" name="name" class="form-control" />
+                        <input type="text" name="name" class="form-control" value="{{ old('name') }}"/>
                     </div>
                     <div class="form-group">
                         <label class="control-label">Description</label>
-                        <textarea name="description" class="form-control" rows="3"></textarea>
+                        <textarea name="description" class="form-control" rows="3">{{ old('description') }}</textarea>
                     </div>
                     <div class="row">
                         <div class="form-group col-md-6">
                             <label class="control-label">Environment Variable <span class="field-required"></span></label>
-                            <input type="text" name="env_variable" class="form-control" />
+                            <input type="text" name="env_variable" class="form-control" value="{{ old('env_variable') }}" />
                         </div>
                         <div class="form-group col-md-6">
                             <label class="control-label">Default Value</label>
-                            <input type="text" name="default_value" class="form-control" />
+                            <input type="text" name="default_value" class="form-control" value="{{ old('default_value') }}" />
                         </div>
                         <div class="col-xs-12">
                             <p class="text-muted small">This variable can be accessed in the statup command by entering <code>@{{environment variable value}}</code>.</p>
@@ -133,7 +133,7 @@
                     </div>
                     <div class="form-group">
                         <label class="control-label">Input Rules <span class="field-required"></span></label>
-                        <input type="text" name="rules" class="form-control" value="required|string|max:20" placeholder="required|string|max:20" />
+                        <input type="text" name="rules" class="form-control" value="{{ old('rules', 'required|string|max:20') }}" placeholder="required|string|max:20" />
                         <p class="text-muted small">These rules are defined using standard Laravel Framework validation rules.</p>
                     </div>
                 </div>

tests/Unit/Services/Eggs/Variables/VariableCreationServiceTest.php

@@ -4,7 +4,9 @@ namespace Tests\Unit\Services\Eggs\Variables;
 
 use Mockery as m;
 use Tests\TestCase;
+use BadMethodCallException;
 use Pterodactyl\Models\EggVariable;
+use Illuminate\Contracts\Validation\Factory;
 use Pterodactyl\Services\Eggs\Variables\VariableCreationService;
 use Pterodactyl\Contracts\Repository\EggVariableRepositoryInterface;
 
@@ -13,12 +15,12 @@ class VariableCreationServiceTest extends TestCase
     /**
      * @var \Pterodactyl\Contracts\Repository\EggVariableRepositoryInterface|\Mockery\Mock
      */
-    protected $repository;
+    private $repository;
 
     /**
-     * @var \Pterodactyl\Services\Eggs\Variables\VariableCreationService
+     * @var \Illuminate\Contracts\Validation\Factory|\Mockery\Mock
      */
-    protected $service;
+    private $validator;
 
     /**
      * Setup tests.
@@ -28,8 +30,7 @@ class VariableCreationServiceTest extends TestCase
         parent::setUp();
 
         $this->repository = m::mock(EggVariableRepositoryInterface::class);
-
+        $this->validator = m::mock(Factory::class);
-        $this->service = new VariableCreationService($this->repository);
     }
 
     /**
@@ -46,7 +47,7 @@ class VariableCreationServiceTest extends TestCase
             'env_variable' => 'TEST_VAR_123',
         ]))->once()->andReturn(new EggVariable);
 
-        $this->assertInstanceOf(EggVariable::class, $this->service->handle(1, $data));
+        $this->assertInstanceOf(EggVariable::class, $this->getService()->handle(1, $data));
     }
 
     /**
@@ -62,7 +63,7 @@ class VariableCreationServiceTest extends TestCase
             'env_variable' => 'TEST_VAR_123',
         ]))->once()->andReturn(new EggVariable);
 
-        $this->assertInstanceOf(EggVariable::class, $this->service->handle(1, $data));
+        $this->assertInstanceOf(EggVariable::class, $this->getService()->handle(1, $data));
     }
 
     /**
@@ -81,18 +82,20 @@ class VariableCreationServiceTest extends TestCase
             'user_editable' => false,
         ]))->once()->andReturn(new EggVariable);
 
-        $this->assertInstanceOf(EggVariable::class, $this->service->handle(1, $data));
+        $this->assertInstanceOf(EggVariable::class, $this->getService()->handle(1, $data));
     }
 
     /**
      * Test that all of the reserved variables defined in the model trigger an exception.
      *
+     * @param string $variable
+     *
      * @dataProvider reservedNamesProvider
      * @expectedException \Pterodactyl\Exceptions\Service\Egg\Variable\ReservedVariableNameException
      */
     public function testExceptionIsThrownIfEnvironmentVariableIsInListOfReservedNames(string $variable)
     {
-        $this->service->handle(1, ['env_variable' => $variable]);
+        $this->getService()->handle(1, ['env_variable' => $variable]);
     }
 
     /**
@@ -106,7 +109,49 @@ class VariableCreationServiceTest extends TestCase
             'egg_id' => 1,
         ]))->once()->andReturn(new EggVariable);
 
-        $this->assertInstanceOf(EggVariable::class, $this->service->handle(1, $data));
+        $this->assertInstanceOf(EggVariable::class, $this->getService()->handle(1, $data));
+    }
+
+    /**
+     * Test that validation errors due to invalid rules are caught and handled properly.
+     *
+     * @expectedException \Pterodactyl\Exceptions\Service\Egg\Variable\BadValidationRuleException
+     * @expectedExceptionMessage The validation rule "hodor_door" is not a valid rule for this application.
+     */
+    public function testInvalidValidationRulesResultInException()
+    {
+        $data = ['env_variable' => 'TEST_VAR_123', 'rules' => 'string|hodorDoor'];
+
+        $this->validator->shouldReceive('make')->once()
+            ->with(['__TEST' => 'test'], ['__TEST' => 'string|hodorDoor'])
+            ->andReturnSelf();
+
+        $this->validator->shouldReceive('fails')->once()
+            ->withNoArgs()
+            ->andThrow(new BadMethodCallException('Method [validateHodorDoor] does not exist.'));
+
+        $this->getService()->handle(1, $data);
+    }
+
+    /**
+     * Test that an exception not stemming from a bad rule is not caught.
+     *
+     * @expectedException \BadMethodCallException
+     * @expectedExceptionMessage Received something, but no expectations were specified.
+     */
+    public function testExceptionNotCausedByBadRuleIsNotCaught()
+    {
+        $data = ['env_variable' => 'TEST_VAR_123', 'rules' => 'string'];
+
+        $this->validator->shouldReceive('make')->once()
+            ->with(['__TEST' => 'test'], ['__TEST' => 'string'])
+            ->andReturnSelf();
+
+        $this->validator->shouldReceive('fails')->once()
+            ->withNoArgs()
+            ->andThrow(new BadMethodCallException('Received something, but no expectations were specified.'));
+
+        $this->getService()->handle(1, $data);
     }
 
     /**
@@ -124,4 +169,14 @@ class VariableCreationServiceTest extends TestCase
 
         return $data;
     }
+
+    /**
+     * Return an instance of the service with mocked dependencies for testing.
+     *
+     * @return \Pterodactyl\Services\Eggs\Variables\VariableCreationService
+     */
+    private function getService(): VariableCreationService
+    {
+        return new VariableCreationService($this->repository, $this->validator);
+    }
 }

tests/Unit/Services/Eggs/Variables/VariableUpdateServiceTest.php

@@ -5,7 +5,9 @@ namespace Tests\Unit\Services\Eggs\Variables;
 use Exception;
 use Mockery as m;
 use Tests\TestCase;
+use BadMethodCallException;
 use Pterodactyl\Models\EggVariable;
+use Illuminate\Contracts\Validation\Factory;
 use Pterodactyl\Exceptions\DisplayException;
 use Pterodactyl\Services\Eggs\Variables\VariableUpdateService;
 use Pterodactyl\Contracts\Repository\EggVariableRepositoryInterface;
@@ -15,17 +17,17 @@ class VariableUpdateServiceTest extends TestCase
     /**
      * @var \Pterodactyl\Models\EggVariable|\Mockery\Mock
      */
-    protected $model;
+    private $model;
 
     /**
      * @var \Pterodactyl\Contracts\Repository\EggVariableRepositoryInterface|\Mockery\Mock
      */
-    protected $repository;
+    private $repository;
 
     /**
-     * @var \Pterodactyl\Services\Eggs\Variables\VariableUpdateService
+     * @var \Illuminate\Contracts\Validation\Factory|\Mockery\Mock
      */
-    protected $service;
+    private $validator;
 
     /**
      * Setup tests.
@@ -36,8 +38,7 @@ class VariableUpdateServiceTest extends TestCase
 
         $this->model = factory(EggVariable::class)->make();
         $this->repository = m::mock(EggVariableRepositoryInterface::class);
-
+        $this->validator = m::mock(Factory::class);
-        $this->service = new VariableUpdateService($this->repository);
     }
 
     /**
@@ -51,7 +52,7 @@ class VariableUpdateServiceTest extends TestCase
                 'user_editable' => false,
             ]))->once()->andReturn(true);
 
-        $this->assertTrue($this->service->handle($this->model, []));
+        $this->assertTrue($this->getService()->handle($this->model, []));
     }
 
     /**
@@ -67,7 +68,7 @@ class VariableUpdateServiceTest extends TestCase
             'default_value' => '',
         ]))->once()->andReturn(true);
 
-        $this->assertTrue($this->service->handle($this->model, ['default_value' => null]));
+        $this->assertTrue($this->getService()->handle($this->model, ['default_value' => null]));
     }
 
     /**
@@ -89,7 +90,7 @@ class VariableUpdateServiceTest extends TestCase
                 'env_variable' => 'TEST_VAR_123',
             ]))->once()->andReturn(true);
 
-        $this->assertTrue($this->service->handle($this->model, ['env_variable' => 'TEST_VAR_123']));
+        $this->assertTrue($this->getService()->handle($this->model, ['env_variable' => 'TEST_VAR_123']));
     }
 
     /**
@@ -107,7 +108,7 @@ class VariableUpdateServiceTest extends TestCase
                 'description' => '',
             ]))->once()->andReturn(true);
 
-        $this->assertTrue($this->service->handle($this->model, ['options' => null, 'description' => null]));
+        $this->assertTrue($this->getService()->handle($this->model, ['options' => null, 'description' => null]));
     }
 
     /**
@@ -129,7 +130,7 @@ class VariableUpdateServiceTest extends TestCase
                 'env_variable' => 'TEST_VAR_123',
             ]))->once()->andReturn(true);
 
-        $this->assertTrue($this->service->handle($this->model, ['user_viewable' => 123456, 'env_variable' => 'TEST_VAR_123']));
+        $this->assertTrue($this->getService()->handle($this->model, ['user_viewable' => 123456, 'env_variable' => 'TEST_VAR_123']));
     }
 
     /**
@@ -145,7 +146,7 @@ class VariableUpdateServiceTest extends TestCase
             ])->once()->andReturn(1);
 
         try {
-            $this->service->handle($this->model, ['env_variable' => 'TEST_VAR_123']);
+            $this->getService()->handle($this->model, ['env_variable' => 'TEST_VAR_123']);
         } catch (Exception $exception) {
             $this->assertInstanceOf(DisplayException::class, $exception);
             $this->assertEquals(trans('exceptions.service.variables.env_not_unique', [
@@ -162,7 +163,51 @@ class VariableUpdateServiceTest extends TestCase
      */
     public function testExceptionIsThrownIfEnvironmentVariableIsInListOfReservedNames(string $variable)
     {
-        $this->service->handle($this->model, ['env_variable' => $variable]);
+        $this->getService()->handle($this->model, ['env_variable' => $variable]);
+    }
+
+    /**
+     * Test that validation errors due to invalid rules are caught and handled properly.
+     *
+     * @expectedException \Pterodactyl\Exceptions\Service\Egg\Variable\BadValidationRuleException
+     * @expectedExceptionMessage The validation rule "hodor_door" is not a valid rule for this application.
+     */
+    public function testInvalidValidationRulesResultInException()
+    {
+        $data = ['env_variable' => 'TEST_VAR_123', 'rules' => 'string|hodorDoor'];
+
+        $this->repository->shouldReceive('setColumns->findCountWhere')->once()->andReturn(0);
+
+        $this->validator->shouldReceive('make')->once()
+            ->with(['__TEST' => 'test'], ['__TEST' => 'string|hodorDoor'])
+            ->andReturnSelf();
+
+        $this->validator->shouldReceive('fails')->once()
+            ->withNoArgs()
+            ->andThrow(new BadMethodCallException('Method [validateHodorDoor] does not exist.'));
+
+        $this->getService()->handle($this->model, $data);
+    }
+
+    /**
+     * Test that an exception not stemming from a bad rule is not caught.
+     *
+     * @expectedException \BadMethodCallException
+     * @expectedExceptionMessage Received something, but no expectations were specified.
+     */
+    public function testExceptionNotCausedByBadRuleIsNotCaught()
+    {
+        $data = ['rules' => 'string'];
+
+        $this->validator->shouldReceive('make')->once()
+            ->with(['__TEST' => 'test'], ['__TEST' => 'string'])
+            ->andReturnSelf();
+
+        $this->validator->shouldReceive('fails')->once()
+            ->withNoArgs()
+            ->andThrow(new BadMethodCallException('Received something, but no expectations were specified.'));
+
+        $this->getService()->handle($this->model, $data);
     }
 
     /**
@@ -180,4 +225,14 @@ class VariableUpdateServiceTest extends TestCase
 
         return $data;
     }
+
+    /**
+     * Return an instance of the service with mocked dependencies for testing.
+     *
+     * @return \Pterodactyl\Services\Eggs\Variables\VariableUpdateService
+     */
+    private function getService(): VariableUpdateService
+    {
+        return new VariableUpdateService($this->repository, $this->validator);
+    }
 }