From 0621d8475d6d702a4892dac1b9a8228b46a41ae4 Mon Sep 17 00:00:00 2001 From: DaneEveritt Date: Sun, 29 May 2022 17:52:14 -0400 Subject: [PATCH] Return tests to passing now that we don't ignore a critical event... --- app/Models/Model.php | 48 ++++++++----------- .../Activity/ActivityLogTargetableService.php | 9 ---- database/Factories/ServerFactory.php | 4 +- database/Factories/TaskFactory.php | 10 +--- database/Factories/UserFactory.php | 5 +- .../Users/ExternalUserControllerTest.php | 5 +- .../Api/Client/AccountControllerTest.php | 5 +- .../Schedule/DeleteServerScheduleTest.php | 2 +- .../Schedule/GetServerSchedulesTest.php | 2 +- .../Schedule/UpdateServerScheduleTest.php | 2 +- .../CreateServerScheduleTaskTest.php | 2 +- .../Server/Subuser/DeleteSubuserTest.php | 3 +- tests/Integration/TestResponse.php | 6 ++- .../Traits/Integration/CreatesTestModels.php | 1 - 14 files changed, 44 insertions(+), 60 deletions(-) diff --git a/app/Models/Model.php b/app/Models/Model.php index 23cf5e7e9..cc8ace6c3 100644 --- a/app/Models/Model.php +++ b/app/Models/Model.php @@ -7,6 +7,7 @@ use Illuminate\Support\Str; use Illuminate\Validation\Rule; use Illuminate\Container\Container; use Illuminate\Contracts\Validation\Factory; +use Illuminate\Validation\ValidationException; use Illuminate\Database\Eloquent\Factories\HasFactory; use Pterodactyl\Exceptions\Model\DataValidationException; use Illuminate\Database\Eloquent\Model as IlluminateModel; @@ -30,13 +31,6 @@ abstract class Model extends IlluminateModel */ protected $skipValidation = false; - /** - * The validator instance used by this model. - * - * @var \Illuminate\Validation\Validator - */ - protected $validator; - /** * @var \Illuminate\Contracts\Validation\Factory */ @@ -60,8 +54,10 @@ abstract class Model extends IlluminateModel static::$validatorFactory = Container::getInstance()->make(Factory::class); static::saving(function (Model $model) { - if (!$model->validate()) { - throw new DataValidationException($model->getValidator()); + try { + $model->validate(); + } catch (ValidationException $exception) { + throw new DataValidationException($exception->validator); } return true; @@ -101,14 +97,9 @@ abstract class Model extends IlluminateModel */ public function getValidator() { - $rules = $this->getKey() ? static::getRulesForUpdate($this) : static::getRules(); + $rules = $this->exists ? static::getRulesForUpdate($this) : static::getRules(); - return $this->validator ?: $this->validator = static::$validatorFactory->make( - [], - $rules, - [], - [] - ); + return static::$validatorFactory->make([], $rules, [], []); } /** @@ -139,14 +130,14 @@ abstract class Model extends IlluminateModel * Returns the rules associated with the model, specifically for updating the given model * rather than just creating it. * - * @param \Illuminate\Database\Eloquent\Model|int|string $id + * @param \Illuminate\Database\Eloquent\Model|int|string $model * * @return array */ - public static function getRulesForUpdate($id, string $primaryKey = 'id') + public static function getRulesForUpdate($model, string $column = 'id') { - if ($id instanceof Model) { - [$primaryKey, $id] = [$id->getKeyName(), $id->getKey()]; + if ($model instanceof Model) { + [$id, $column] = [$model->getKey(), $model->getKeyName()]; } $rules = static::getRules(); @@ -163,7 +154,7 @@ abstract class Model extends IlluminateModel [, $args] = explode(':', $datum); $args = explode(',', $args); - $datum = Rule::unique($args[0], $args[1] ?? $key)->ignore($id, $primaryKey)->__toString(); + $datum = Rule::unique($args[0], $args[1] ?? $key)->ignore($id ?? $model, $column); } } @@ -172,16 +163,15 @@ abstract class Model extends IlluminateModel /** * Determines if the model is in a valid state or not. - * - * @return bool */ - public function validate() + public function validate(): void { if ($this->skipValidation) { - return true; + return; } - return $this->getValidator()->setData( + $validator = $this->getValidator(); + $validator->setData( // Trying to do self::toArray() here will leave out keys based on the whitelist/blacklist // for that model. Doing this will return all of the attributes in a format that can // properly be validated. @@ -189,7 +179,11 @@ abstract class Model extends IlluminateModel $this->getAttributes(), $this->getMutatedAttributes() ) - )->passes(); + ); + + if (!$validator->passes()) { + throw new ValidationException($validator); + } } /** diff --git a/app/Services/Activity/ActivityLogTargetableService.php b/app/Services/Activity/ActivityLogTargetableService.php index 6883a4ce1..a4da5b5f3 100644 --- a/app/Services/Activity/ActivityLogTargetableService.php +++ b/app/Services/Activity/ActivityLogTargetableService.php @@ -2,7 +2,6 @@ namespace Pterodactyl\Services\Activity; -use InvalidArgumentException; use Illuminate\Database\Eloquent\Model; class ActivityLogTargetableService @@ -13,19 +12,11 @@ class ActivityLogTargetableService public function setActor(Model $actor): void { - if (!is_null($this->actor)) { - throw new InvalidArgumentException('Cannot call ' . __METHOD__ . ' when an actor is already set on the instance.'); - } - $this->actor = $actor; } public function setSubject(Model $subject): void { - if (!is_null($this->subject)) { - throw new InvalidArgumentException('Cannot call ' . __METHOD__ . ' when a target is already set on the instance.'); - } - $this->subject = $subject; } diff --git a/database/Factories/ServerFactory.php b/database/Factories/ServerFactory.php index b9145c679..d89bd6a56 100644 --- a/database/Factories/ServerFactory.php +++ b/database/Factories/ServerFactory.php @@ -40,8 +40,8 @@ class ServerFactory extends Factory 'oom_disabled' => 0, 'startup' => '/bin/bash echo "hello world"', 'image' => 'foo/bar:latest', - 'allocation_limit' => 0, - 'database_limit' => 0, + 'allocation_limit' => null, + 'database_limit' => null, 'backup_limit' => 0, 'created_at' => Carbon::now(), 'updated_at' => Carbon::now(), diff --git a/database/Factories/TaskFactory.php b/database/Factories/TaskFactory.php index f04ebbe8b..32bf950cc 100644 --- a/database/Factories/TaskFactory.php +++ b/database/Factories/TaskFactory.php @@ -2,25 +2,17 @@ namespace Database\Factories; -use Pterodactyl\Models\Task; use Illuminate\Database\Eloquent\Factories\Factory; class TaskFactory extends Factory { - /** - * The name of the factory's corresponding model. - * - * @var string - */ - protected $model = Task::class; - /** * Define the model's default state. */ public function definition(): array { return [ - 'sequence_id' => $this->faker->randomNumber(1), + 'sequence_id' => $this->faker->numberBetween(1, 10), 'action' => 'command', 'payload' => 'test command', 'time_offset' => 120, diff --git a/database/Factories/UserFactory.php b/database/Factories/UserFactory.php index 8eae7c55d..cabfed7be 100644 --- a/database/Factories/UserFactory.php +++ b/database/Factories/UserFactory.php @@ -4,6 +4,7 @@ namespace Database\Factories; use Carbon\Carbon; use Ramsey\Uuid\Uuid; +use Illuminate\Support\Str; use Pterodactyl\Models\User; use Illuminate\Database\Eloquent\Factories\Factory; @@ -24,10 +25,10 @@ class UserFactory extends Factory static $password; return [ - 'external_id' => $this->faker->unique()->isbn10, + 'external_id' => null, 'uuid' => Uuid::uuid4()->toString(), 'username' => $this->faker->unique()->userName, - 'email' => $this->faker->unique()->safeEmail, + 'email' => Str::random(32) . '@example.com', 'name_first' => $this->faker->firstName, 'name_last' => $this->faker->lastName, 'password' => $password ?: $password = bcrypt('password'), diff --git a/tests/Integration/Api/Application/Users/ExternalUserControllerTest.php b/tests/Integration/Api/Application/Users/ExternalUserControllerTest.php index 74e03df54..fc37b7232 100644 --- a/tests/Integration/Api/Application/Users/ExternalUserControllerTest.php +++ b/tests/Integration/Api/Application/Users/ExternalUserControllerTest.php @@ -2,6 +2,7 @@ namespace Pterodactyl\Tests\Integration\Api\Application\Users; +use Illuminate\Support\Str; use Pterodactyl\Models\User; use Illuminate\Http\Response; use Pterodactyl\Tests\Integration\Api\Application\ApplicationApiIntegrationTestCase; @@ -13,7 +14,7 @@ class ExternalUserControllerTest extends ApplicationApiIntegrationTestCase */ public function testGetRemoteUser() { - $user = User::factory()->create(); + $user = User::factory()->create(['external_id' => Str::random()]); $response = $this->getJson('/api/application/users/external/' . $user->external_id); $response->assertStatus(Response::HTTP_OK); @@ -60,7 +61,7 @@ class ExternalUserControllerTest extends ApplicationApiIntegrationTestCase */ public function testErrorReturnedIfNoPermission() { - $user = User::factory()->create(); + $user = User::factory()->create(['external_id' => Str::random()]); $this->createNewDefaultApiKey($this->getApiUser(), ['r_users' => 0]); $response = $this->getJson('/api/application/users/external/' . $user->external_id); diff --git a/tests/Integration/Api/Client/AccountControllerTest.php b/tests/Integration/Api/Client/AccountControllerTest.php index 971b83421..6505f43f1 100644 --- a/tests/Integration/Api/Client/AccountControllerTest.php +++ b/tests/Integration/Api/Client/AccountControllerTest.php @@ -2,6 +2,7 @@ namespace Pterodactyl\Tests\Integration\Api\Client; +use Illuminate\Support\Str; use Pterodactyl\Models\User; use Illuminate\Http\Response; use Illuminate\Support\Facades\Hash; @@ -41,13 +42,13 @@ class AccountControllerTest extends ClientApiIntegrationTestCase $user = User::factory()->create(); $response = $this->actingAs($user)->putJson('/api/client/account/email', [ - 'email' => 'hodor@example.com', + 'email' => $email = Str::random() . '@example.com', 'password' => 'password', ]); $response->assertStatus(Response::HTTP_NO_CONTENT); - $this->assertDatabaseHas('users', ['id' => $user->id, 'email' => 'hodor@example.com']); + $this->assertDatabaseHas('users', ['id' => $user->id, 'email' => $email]); } /** diff --git a/tests/Integration/Api/Client/Server/Schedule/DeleteServerScheduleTest.php b/tests/Integration/Api/Client/Server/Schedule/DeleteServerScheduleTest.php index 25f274115..f5a2e49dd 100644 --- a/tests/Integration/Api/Client/Server/Schedule/DeleteServerScheduleTest.php +++ b/tests/Integration/Api/Client/Server/Schedule/DeleteServerScheduleTest.php @@ -50,7 +50,7 @@ class DeleteServerScheduleTest extends ClientApiIntegrationTestCase public function testNotFoundErrorIsReturnedIfScheduleDoesNotBelongToServer() { [$user, $server] = $this->generateTestAccount(); - [, $server2] = $this->generateTestAccount(['user_id' => $user->id]); + $server2 = $this->createServerModel(['owner_id' => $user->id]); $schedule = Schedule::factory()->create(['server_id' => $server2->id]); diff --git a/tests/Integration/Api/Client/Server/Schedule/GetServerSchedulesTest.php b/tests/Integration/Api/Client/Server/Schedule/GetServerSchedulesTest.php index 57c282ee7..34830a421 100644 --- a/tests/Integration/Api/Client/Server/Schedule/GetServerSchedulesTest.php +++ b/tests/Integration/Api/Client/Server/Schedule/GetServerSchedulesTest.php @@ -64,7 +64,7 @@ class GetServerSchedulesTest extends ClientApiIntegrationTestCase public function testScheduleBelongingToAnotherServerCannotBeViewed() { [$user, $server] = $this->generateTestAccount(); - [, $server2] = $this->generateTestAccount(['user_id' => $user->id]); + $server2 = $this->createServerModel(['owner_id' => $user->id]); $schedule = Schedule::factory()->create(['server_id' => $server2->id]); diff --git a/tests/Integration/Api/Client/Server/Schedule/UpdateServerScheduleTest.php b/tests/Integration/Api/Client/Server/Schedule/UpdateServerScheduleTest.php index e99486d4e..f095dd88f 100644 --- a/tests/Integration/Api/Client/Server/Schedule/UpdateServerScheduleTest.php +++ b/tests/Integration/Api/Client/Server/Schedule/UpdateServerScheduleTest.php @@ -58,7 +58,7 @@ class UpdateServerScheduleTest extends ClientApiIntegrationTestCase public function testErrorIsReturnedIfScheduleDoesNotBelongToServer() { [$user, $server] = $this->generateTestAccount(); - [, $server2] = $this->generateTestAccount(['user_id' => $user->id]); + $server2 = $this->createServerModel(['owner_id' => $user->id]); $schedule = Schedule::factory()->create(['server_id' => $server2->id]); diff --git a/tests/Integration/Api/Client/Server/ScheduleTask/CreateServerScheduleTaskTest.php b/tests/Integration/Api/Client/Server/ScheduleTask/CreateServerScheduleTaskTest.php index b031b4fb2..81c65b1d2 100644 --- a/tests/Integration/Api/Client/Server/ScheduleTask/CreateServerScheduleTaskTest.php +++ b/tests/Integration/Api/Client/Server/ScheduleTask/CreateServerScheduleTaskTest.php @@ -145,7 +145,7 @@ class CreateServerScheduleTaskTest extends ClientApiIntegrationTestCase public function testErrorIsReturnedIfScheduleDoesNotBelongToServer() { [$user, $server] = $this->generateTestAccount(); - [, $server2] = $this->generateTestAccount(['user_id' => $user->id]); + $server2 = $this->createServerModel(['owner_id' => $user->id]); /** @var \Pterodactyl\Models\Schedule $schedule */ $schedule = Schedule::factory()->create(['server_id' => $server2->id]); diff --git a/tests/Integration/Api/Client/Server/Subuser/DeleteSubuserTest.php b/tests/Integration/Api/Client/Server/Subuser/DeleteSubuserTest.php index eeb248510..fbcbbc01c 100644 --- a/tests/Integration/Api/Client/Server/Subuser/DeleteSubuserTest.php +++ b/tests/Integration/Api/Client/Server/Subuser/DeleteSubuserTest.php @@ -32,8 +32,9 @@ class DeleteSubuserTest extends ClientApiIntegrationTestCase /** @var \Pterodactyl\Models\User $differentUser */ $differentUser = User::factory()->create(); + $real = Uuid::uuid4()->toString(); // Generate a UUID that lines up with a user in the database if it were to be cast to an int. - $uuid = $differentUser->id . str_repeat('a', strlen((string) $differentUser->id)) . substr(Uuid::uuid4()->toString(), 8); + $uuid = $differentUser->id . substr($real, strlen((string) $differentUser->id)); /** @var \Pterodactyl\Models\User $subuser */ $subuser = User::factory()->create(['uuid' => $uuid]); diff --git a/tests/Integration/TestResponse.php b/tests/Integration/TestResponse.php index 9553f432a..e715071e9 100644 --- a/tests/Integration/TestResponse.php +++ b/tests/Integration/TestResponse.php @@ -28,7 +28,11 @@ class TestResponse extends IlluminateTestResponse if ($actual !== $status && $status !== 500) { $this->dump(); if (!is_null($this->exception) && !$this->exception instanceof DisplayException && !$this->exception instanceof ValidationException) { - dump($this->exception); + dump([ + 'exception_class' => get_class($this->exception), + 'message' => $this->exception->getMessage(), + 'trace' => $this->exception->getTrace(), + ]); } } diff --git a/tests/Traits/Integration/CreatesTestModels.php b/tests/Traits/Integration/CreatesTestModels.php index 51e4b76aa..ce64376ce 100644 --- a/tests/Traits/Integration/CreatesTestModels.php +++ b/tests/Traits/Integration/CreatesTestModels.php @@ -94,7 +94,6 @@ trait CreatesTestModels return [$user, $this->createServerModel(['user_id' => $user->id])]; } - /** @var \Pterodactyl\Models\Server $server */ $server = $this->createServerModel(); Subuser::query()->create([