Fix issues with validation in admin CP for server variables, closes pterodactyl#780

DaneEveritt · DaneEveritt · commit 20c1c7411676 · 2017-11-25T13:15:46.000-06:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,7 @@ This project follows [Semantic Versioning](http://semver.org) guidelines.
 ### Fixed
 * `[beta.2]` — Fixes a bug that would cause an endless exception message stream in the console when attemping to setup environment settings in certain instances.
 * `[beta.2]` — Fixes a bug causing the dropdown menu for a server's egg to display the wrong selected value.
+* `[beta.2]` — Fixes a bug that would throw a red page of death when submitting an invalid egg variable value for a server in the Admin CP.
 
 ## v0.7.0-beta.2 (Derelict Dermodactylus)
 ### Fixed
diff --git a/app/Exceptions/DisplayValidationException.php b/app/Exceptions/DisplayValidationException.php
@@ -9,6 +9,6 @@
 
 namespace Pterodactyl\Exceptions;
 
-class DisplayValidationException extends PterodactylException
+class DisplayValidationException extends DisplayException
 {
 }
diff --git a/app/Services/Servers/StartupModificationService.php b/app/Services/Servers/StartupModificationService.php
@@ -78,9 +78,10 @@ public function __construct(
      * @param \Pterodactyl\Models\Server $server
      * @param array                      $data
      *
-     * @throws \Pterodactyl\Exceptions\DisplayException
+     * @throws \Illuminate\Validation\ValidationException
      * @throws \Pterodactyl\Exceptions\Model\DataValidationException
      * @throws \Pterodactyl\Exceptions\Repository\RecordNotFoundException
+     * @throws \Pterodactyl\Exceptions\Http\Connection\DaemonConnectionException
      */
     public function handle(Server $server, array $data)
     {
diff --git a/app/Services/Servers/VariableValidatorService.php b/app/Services/Servers/VariableValidatorService.php
@@ -11,8 +11,8 @@
 
 use Pterodactyl\Models\User;
 use Illuminate\Support\Collection;
+use Illuminate\Validation\ValidationException;
 use Pterodactyl\Traits\Services\HasUserLevels;
-use Pterodactyl\Exceptions\DisplayValidationException;
 use Pterodactyl\Contracts\Repository\ServerRepositoryInterface;
 use Illuminate\Contracts\Validation\Factory as ValidationFactory;
 use Pterodactyl\Contracts\Repository\EggVariableRepositoryInterface;
@@ -25,22 +25,22 @@ class VariableValidatorService
     /**
      * @var \Pterodactyl\Contracts\Repository\EggVariableRepositoryInterface
      */
-    protected $optionVariableRepository;
+    private $optionVariableRepository;
 
     /**
      * @var \Pterodactyl\Contracts\Repository\ServerRepositoryInterface
      */
-    protected $serverRepository;
+    private $serverRepository;
 
     /**
      * @var \Pterodactyl\Contracts\Repository\ServerVariableRepositoryInterface
      */
-    protected $serverVariableRepository;
+    private $serverVariableRepository;
 
     /**
      * @var \Illuminate\Contracts\Validation\Factory
      */
-    protected $validator;
+    private $validator;
 
     /**
      * VariableValidatorService constructor.
@@ -68,32 +68,32 @@ public function __construct(
      * @param int   $egg
      * @param array $fields
      * @return \Illuminate\Support\Collection
+     * @throws \Illuminate\Validation\ValidationException
      */
     public function handle(int $egg, array $fields = []): Collection
     {
         $variables = $this->optionVariableRepository->findWhere([['egg_id', '=', $egg]]);
+        $messages = $this->validator->make([], []);
 
-        return $variables->map(function ($item) use ($fields) {
+        $response = $variables->map(function ($item) use ($fields, $messages) {
             // Skip doing anything if user is not an admin and
             // variable is not user viewable or editable.
             if (! $this->isUserLevel(User::USER_LEVEL_ADMIN) && (! $item->user_editable || ! $item->user_viewable)) {
                 return false;
             }
 
-            $validator = $this->validator->make([
+            $v = $this->validator->make([
                 'variable_value' => array_get($fields, $item->env_variable),
             ], [
                 'variable_value' => $item->rules,
+            ], [], [
+                'variable_value' => trans('validation.internal.variable_value', ['env' => $item->name]),
             ]);
 
-            if ($validator->fails()) {
-                throw new DisplayValidationException(json_encode(
-                    collect([
-                        'notice' => [
-                            trans('admin/server.exceptions.bad_variable', ['name' => $item->name]),
-                        ],
-                    ])->merge($validator->errors()->toArray())
-                ));
+            if ($v->fails()) {
+                foreach ($v->getMessageBag()->all() as $message) {
+                    $messages->getMessageBag()->add($item->env_variable, $message);
+                }
             }
 
             return (object) [
@@ -104,5 +104,11 @@ public function handle(int $egg, array $fields = []): Collection
         })->filter(function ($item) {
             return is_object($item);
         });
+
+        if (! empty($messages->getMessageBag()->all())) {
+            throw new ValidationException($messages);
+        }
+
+        return $response;
     }
 }
diff --git a/resources/lang/en/validation.php b/resources/lang/en/validation.php
@@ -85,23 +85,6 @@
     'uploaded' => 'The :attribute failed to upload.',
     'url' => 'The :attribute format is invalid.',
 
-    /*
-    |--------------------------------------------------------------------------
-    | Custom Validation Language Lines
-    |--------------------------------------------------------------------------
-    |
-    | Here you may specify custom validation messages for attributes using the
-    | convention "attribute.rule" to name the lines. This makes it quick to
-    | specify a specific custom language line for a given attribute rule.
-    |
-    */
-
-    'custom' => [
-        'attribute-name' => [
-            'rule-name' => 'custom-message',
-        ],
-    ],
-
     /*
     |--------------------------------------------------------------------------
     | Custom Validation Attributes
@@ -114,4 +97,9 @@
     */
 
     'attributes' => [],
+
+    // Internal validation logic for Pterodactyl
+    'internal' => [
+        'variable_value' => ':env variable',
+    ],
 ];
diff --git a/tests/Unit/Services/Servers/VariableValidatorServiceTest.php b/tests/Unit/Services/Servers/VariableValidatorServiceTest.php
@@ -1,11 +1,4 @@
 <?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\Servers;
 
@@ -15,8 +8,7 @@
 use Illuminate\Support\Collection;
 use Pterodactyl\Models\EggVariable;
 use Illuminate\Contracts\Validation\Factory;
-use Pterodactyl\Exceptions\PterodactylException;
-use Pterodactyl\Exceptions\DisplayValidationException;
+use Illuminate\Validation\ValidationException;
 use Pterodactyl\Services\Servers\VariableValidatorService;
 use Pterodactyl\Contracts\Repository\ServerRepositoryInterface;
 use Pterodactyl\Contracts\Repository\EggVariableRepositoryInterface;
@@ -27,22 +19,17 @@ class VariableValidatorServiceTest extends TestCase
     /**
      * @var \Pterodactyl\Contracts\Repository\EggVariableRepositoryInterface|\Mockery\Mock
      */
-    protected $optionVariableRepository;
+    private $optionVariableRepository;
 
     /**
      * @var \Pterodactyl\Contracts\Repository\ServerRepositoryInterface|\Mockery\Mock
      */
-    protected $serverRepository;
+    private $serverRepository;
 
     /**
      * @var \Pterodactyl\Contracts\Repository\ServerVariableRepositoryInterface|\Mockery\Mock
      */
-    protected $serverVariableRepository;
-
-    /**
-     * @var \Illuminate\Contracts\Validation\Factory|\Mockery\Mock
-     */
-    protected $validator;
+    private $serverVariableRepository;
 
     /**
      * Setup tests.
@@ -54,7 +41,6 @@ public function setUp()
         $this->optionVariableRepository = m::mock(EggVariableRepositoryInterface::class);
         $this->serverRepository = m::mock(ServerRepositoryInterface::class);
         $this->serverVariableRepository = m::mock(ServerVariableRepositoryInterface::class);
-        $this->validator = m::mock(Factory::class);
     }
 
     /**
@@ -77,13 +63,6 @@ public function testValidatorShouldNotProcessVariablesSetAsNotUserEditableWhenAd
         $variables = $this->getVariableCollection();
         $this->optionVariableRepository->shouldReceive('findWhere')->with([['egg_id', '=', 1]])->andReturn($variables);
 
-        $this->validator->shouldReceive('make')->with([
-            'variable_value' => 'Test_SomeValue_0',
-        ], [
-            'variable_value' => $variables[0]->rules,
-        ])->once()->andReturnSelf();
-        $this->validator->shouldReceive('fails')->withNoArgs()->once()->andReturn(false);
-
         $response = $this->getService()->handle(1, [
             $variables[0]->env_variable => 'Test_SomeValue_0',
             $variables[1]->env_variable => 'Test_SomeValue_1',
@@ -112,15 +91,6 @@ public function testValidatorShouldProcessAllVariablesWhenAdminFlagIsSet()
         $variables = $this->getVariableCollection();
         $this->optionVariableRepository->shouldReceive('findWhere')->with([['egg_id', '=', 1]])->andReturn($variables);
 
-        foreach ($variables as $key => $variable) {
-            $this->validator->shouldReceive('make')->with([
-                'variable_value' => 'Test_SomeValue_' . $key,
-            ], [
-                'variable_value' => $variables[$key]->rules,
-            ])->once()->andReturnSelf();
-            $this->validator->shouldReceive('fails')->withNoArgs()->once()->andReturn(false);
-        }
-
         $service = $this->getService();
         $service->setUserLevel(User::USER_LEVEL_ADMIN);
         $response = $service->handle(1, [
@@ -152,28 +122,16 @@ public function testValidatorShouldThrowExceptionWhenAValidationErrorIsEncounter
         $variables = $this->getVariableCollection();
         $this->optionVariableRepository->shouldReceive('findWhere')->with([['egg_id', '=', 1]])->andReturn($variables);
 
-        $this->validator->shouldReceive('make')->with([
-            'variable_value' => null,
-        ], [
-            'variable_value' => $variables[0]->rules,
-        ])->once()->andReturnSelf();
-        $this->validator->shouldReceive('fails')->withNoArgs()->once()->andReturn(true);
-
-        $this->validator->shouldReceive('errors')->withNoArgs()->once()->andReturnSelf();
-        $this->validator->shouldReceive('toArray')->withNoArgs()->once()->andReturn([]);
-
         try {
             $this->getService()->handle(1, [$variables[0]->env_variable => null]);
-        } catch (PterodactylException $exception) {
-            $this->assertInstanceOf(DisplayValidationException::class, $exception);
-
-            $decoded = json_decode($exception->getMessage());
-            $this->assertEquals(0, json_last_error(), 'Assert that response is decodable JSON.');
-            $this->assertObjectHasAttribute('notice', $decoded);
-            $this->assertEquals(
-                trans('admin/server.exceptions.bad_variable', ['name' => $variables[0]->name]),
-                $decoded->notice[0]
-            );
+        } catch (ValidationException $exception) {
+            $messages = $exception->validator->getMessageBag()->all();
+
+            $this->assertNotEmpty($messages);
+            $this->assertSame(1, count($messages));
+            $this->assertSame(trans('validation.required', [
+                'attribute' => trans('validation.internal.variable_value', ['env' => $variables[0]->name]),
+            ]), $messages[0]);
         }
     }
 
@@ -205,7 +163,7 @@ private function getService(): VariableValidatorService
             $this->optionVariableRepository,
             $this->serverRepository,
             $this->serverVariableRepository,
-            $this->validator
+            $this->app->make(Factory::class)
         );
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -9,6 +9,6 @@`
`9`	`9`
`10`	`10`	`namespace Pterodactyl\Exceptions;`
`11`	`11`
`12`		`-class DisplayValidationException extends PterodactylException`
	`12`	`+class DisplayValidationException extends DisplayException`
`13`	`13`	`{`
`14`	`14`	`}`
Original file line number	Diff line number	Diff line change
`@@ -78,9 +78,10 @@ public function __construct(`
`78`	`78`	`* @param \Pterodactyl\Models\Server $server`
`79`	`79`	`* @param array $data`
`80`	`80`	`*`
`81`		`- * @throws \Pterodactyl\Exceptions\DisplayException`
	`81`	`+ * @throws \Illuminate\Validation\ValidationException`
`82`	`82`	`* @throws \Pterodactyl\Exceptions\Model\DataValidationException`
`83`	`83`	`* @throws \Pterodactyl\Exceptions\Repository\RecordNotFoundException`
	`84`	`+ * @throws \Pterodactyl\Exceptions\Http\Connection\DaemonConnectionException`
`84`	`85`	`*/`
`85`	`86`	`public function handle(Server $server, array $data)`
`86`	`87`	`{`