Fix bug preventing the creation of API keys with CIDR ranges

app/Http/Requests/Api/Client/Account/StoreApiKeyRequest.php

@@ -2,7 +2,9 @@
 
 namespace Pterodactyl\Http\Requests\Api\Client\Account;
 
+use IPTools\Range;
 use Pterodactyl\Models\ApiKey;
+use Illuminate\Validation\Validator;
 use Pterodactyl\Http\Requests\Api\Client\ClientApiRequest;
 
 class StoreApiKeyRequest extends ClientApiRequest
@@ -13,18 +15,33 @@ class StoreApiKeyRequest extends ClientApiRequest
 
         return [
             'description' => $rules['memo'],
-            'allowed_ips' => $rules['allowed_ips'],
+            'allowed_ips' => [...$rules['allowed_ips'], 'max:50'],
-            'allowed_ips.*' => 'ip',
+            'allowed_ips.*' => 'string',
         ];
     }
 
     /**
-     * @return array|string[]
+     * Check that each of the values entered is actually valid.
      */
-    public function messages()
+    public function withValidator(Validator $validator): void
     {
-        return [
+        $validator->after(function (Validator $validator) {
-            'allowed_ips.*' => 'All of the IP addresses entered must be valid IPv4 addresses.',
+            if (!is_array($ips = $this->input('allowed_ips'))) {
-        ];
+                return;
+            }
+
+            foreach ($ips as $index => $ip) {
+                $valid = false;
+                try {
+                    $valid = Range::parse($ip)->valid();
+                } catch (\Exception $exception) {
+                    if ($exception->getMessage() !== 'Invalid IP address format') {
+                        throw $exception;
+                    }
+                } finally {
+                    $validator->errors()->addIf(!$valid, "allowed_ips.{$index}", '"' . $ip . '" is not a valid IP address or CIDR range.');
+                }
+            }
+        });
     }
 }

tests/Assertions/AssertsActivityLogged.php

@@ -0,0 +1,84 @@
+<?php
+
+namespace Pterodactyl\Tests\Assertions;
+
+use PHPUnit\Framework\Assert;
+use Illuminate\Support\Facades\Event;
+use Pterodactyl\Events\ActivityLogged;
+use Illuminate\Database\Eloquent\Model;
+use Pterodactyl\Models\ActivityLogSubject;
+
+trait AssertsActivityLogged
+{
+    /**
+     * @param \Illuminate\Database\Eloquent\Model|array $subjects
+     */
+    public function assertActivityFor(string $event, ?Model $actor, ...$subjects): void
+    {
+        $this->assertActivityLogged($event);
+        $this->assertActivityActor($event, $actor);
+        $this->assertActivitySubjects($event, ...$subjects);
+    }
+
+    /**
+     * Asserts that the given activity log event was stored in the database.
+     */
+    public function assertActivityLogged(string $event): void
+    {
+        Event::assertDispatched(ActivityLogged::class, fn ($e) => $e->is($event));
+    }
+
+    /**
+     * Asserts that a given activity log event was stored with the subjects being
+     * any of the values provided.
+     *
+     * @param \Illuminate\Database\Eloquent\Model|array $subjects
+     */
+    public function assertActivitySubjects(string $event, $subjects): void
+    {
+        if (is_array($subjects)) {
+            \Webmozart\Assert\Assert::lessThanEq(count(func_get_args()), 2, 'Invalid call to ' . __METHOD__ . ': cannot provide additional arguments if providing an array.');
+        } else {
+            $subjects = array_slice(func_get_args(), 1);
+        }
+
+        Event::assertDispatched(ActivityLogged::class, function (ActivityLogged $e) use ($event, $subjects) {
+            Assert::assertEquals($event, $e->model->event);
+            Assert::assertNotEmpty($e->model->subjects);
+
+            foreach ($subjects as $subject) {
+                $match = $e->model->subjects->first(function (ActivityLogSubject $model) use ($subject) {
+                    return $model->subject_type === $subject->getMorphClass()
+                        && $model->subject_id = $subject->getKey();
+                });
+
+                Assert::assertNotNull(
+                    $match,
+                    sprintf('Failed asserting that event "%s" includes a %s[%d] subject', $event, get_class($subject), $subject->getKey())
+                );
+            }
+
+            return true;
+        });
+    }
+
+    /**
+     * Asserts that the provided event was logged into the activity logs with the provided
+     * actor model associated with it.
+     */
+    public function assertActivityActor(string $event, ?Model $actor = null): void
+    {
+        Event::assertDispatched(ActivityLogged::class, function (ActivityLogged $e) use ($event, $actor) {
+            Assert::assertEquals($event, $e->model->event);
+
+            if (is_null($actor)) {
+                Assert::assertNull($e->actor());
+            } else {
+                Assert::assertNotNull($e->actor());
+                Assert::assertTrue($e->actor()->is($actor));
+            }
+
+            return true;
+        });
+    }
+}

tests/Integration/Api/Client/ApiKeyControllerTest.php

@@ -5,6 +5,8 @@ namespace Pterodactyl\Tests\Integration\Api\Client;
 use Pterodactyl\Models\User;
 use Illuminate\Http\Response;
 use Pterodactyl\Models\ApiKey;
+use Illuminate\Support\Facades\Event;
+use Pterodactyl\Events\ActivityLogged;
 
 class ApiKeyControllerTest extends ClientApiIntegrationTestCase
 {
@@ -26,37 +28,26 @@ class ApiKeyControllerTest extends ClientApiIntegrationTestCase
         /** @var \Pterodactyl\Models\User $user */
         $user = User::factory()->create();
         /** @var \Pterodactyl\Models\ApiKey $key */
-        $key = ApiKey::factory()->create([
+        $key = ApiKey::factory()->for($user)->create([
-            'user_id' => $user->id,
             'key_type' => ApiKey::TYPE_ACCOUNT,
         ]);
 
-        $response = $this->actingAs($user)->get('/api/client/account/api-keys');
+        $response = $this->actingAs($user)->get('/api/client/account/api-keys')
+            ->assertOk()
+            ->assertJsonPath('object', 'list')
+            ->assertJsonPath('data.0.object', ApiKey::RESOURCE_NAME);
 
-        $response->assertOk();
+        $this->assertJsonTransformedWith($response->json('data.0.attributes'), $key);
-        $response->assertJson([
-            'object' => 'list',
-            'data' => [
-                [
-                    'object' => 'api_key',
-                    'attributes' => [
-                        'identifier' => $key->identifier,
-                        'description' => $key->memo,
-                        'allowed_ips' => $key->allowed_ips,
-                        'last_used_at' => null,
-                        'created_at' => $key->created_at->toIso8601String(),
-                    ],
-                ],
-            ],
-        ]);
     }
 
     /**
      * Test that an API key can be created for the client account. This also checks that the
      * API key secret is returned as metadata in the response since it will not be returned
      * after that point.
+     *
+     * @dataProvider validIPAddressDataProvider
      */
-    public function testApiKeyCanBeCreatedForAccount()
+    public function testApiKeyCanBeCreatedForAccount(array $data)
     {
         /** @var \Pterodactyl\Models\User $user */
         $user = User::factory()->create();
@@ -71,27 +62,37 @@ class ApiKeyControllerTest extends ClientApiIntegrationTestCase
 
         $response = $this->actingAs($user)->postJson('/api/client/account/api-keys', [
             'description' => 'Test Description',
-            'allowed_ips' => ['127.0.0.1'],
+            'allowed_ips' => $data,
-        ]);
+        ])
-
+            ->assertOk()
-        $response->assertOk();
+            ->assertJsonPath('object', ApiKey::RESOURCE_NAME);
 
         /** @var \Pterodactyl\Models\ApiKey $key */
         $key = ApiKey::query()->where('identifier', $response->json('attributes.identifier'))->firstOrFail();
 
-        $response->assertJson([
+        $this->assertJsonTransformedWith($response->json('attributes'), $key);
-            'object' => 'api_key',
+        $response->assertJsonPath('meta.secret_token', decrypt($key->token));
-            'attributes' => [
+
-                'identifier' => $key->identifier,
+        $this->assertActivityFor('user:api-key.create', $user, [$key, $user]);
-                'description' => 'Test Description',
+    }
-                'allowed_ips' => ['127.0.0.1'],
+
-                'last_used_at' => null,
+    /**
-                'created_at' => $key->created_at->toIso8601String(),
+     * Block requests to create an API key specifying more than 50 IP addresses.
-            ],
+     */
-            'meta' => [
+    public function testApiKeyCannotSpecifyMoreThanFiftyIps()
-                'secret_token' => decrypt($key->token),
+    {
-            ],
+        $ips = [];
-        ]);
+        for ($i = 0; $i < 100; ++$i) {
+            $ips[] = '127.0.0.' . $i;
+        }
+
+        $this->actingAs(User::factory()->create())
+            ->postJson('/api/client/account/api-keys', [
+                'description' => 'Test Data',
+                'allowed_ips' => $ips,
+            ])
+            ->assertUnprocessable()
+            ->assertJsonPath('errors.0.detail', 'The allowed ips may not have more than 50 items.');
     }
 
     /**
@@ -104,19 +105,17 @@ class ApiKeyControllerTest extends ClientApiIntegrationTestCase
     {
         /** @var \Pterodactyl\Models\User $user */
         $user = User::factory()->create();
-        ApiKey::factory()->times(5)->create([
+        ApiKey::factory()->times(5)->for($user)->create([
-            'user_id' => $user->id,
             'key_type' => ApiKey::TYPE_ACCOUNT,
         ]);
 
-        $response = $this->actingAs($user)->postJson('/api/client/account/api-keys', [
+        $this->actingAs($user)->postJson('/api/client/account/api-keys', [
             'description' => 'Test Description',
             'allowed_ips' => ['127.0.0.1'],
-        ]);
+        ])
-
+            ->assertStatus(Response::HTTP_BAD_REQUEST)
-        $response->assertStatus(Response::HTTP_BAD_REQUEST);
+            ->assertJsonPath('errors.0.code', 'DisplayException')
-        $response->assertJsonPath('errors.0.code', 'DisplayException');
+            ->assertJsonPath('errors.0.detail', 'You have reached the account limit for number of API keys.');
-        $response->assertJsonPath('errors.0.detail', 'You have reached the account limit for number of API keys.');
     }
 
     /**
@@ -126,26 +125,33 @@ class ApiKeyControllerTest extends ClientApiIntegrationTestCase
      */
     public function testValidationErrorIsReturnedForBadRequests()
     {
-        /** @var \Pterodactyl\Models\User $user */
+        $this->actingAs(User::factory()->create());
-        $user = User::factory()->create();
 
-        $response = $this->actingAs($user)->postJson('/api/client/account/api-keys', [
+        $this->postJson('/api/client/account/api-keys', [
             'description' => '',
             'allowed_ips' => ['127.0.0.1'],
-        ]);
+        ])
+            ->assertUnprocessable()
+            ->assertJsonPath('errors.0.meta.rule', 'required')
+            ->assertJsonPath('errors.0.detail', 'The description field is required.');
 
-        $response->assertStatus(Response::HTTP_UNPROCESSABLE_ENTITY);
+        $this->postJson('/api/client/account/api-keys', [
-        $response->assertJsonPath('errors.0.meta.rule', 'required');
-        $response->assertJsonPath('errors.0.detail', 'The description field is required.');
-
-        $response = $this->actingAs($user)->postJson('/api/client/account/api-keys', [
             'description' => str_repeat('a', 501),
             'allowed_ips' => ['127.0.0.1'],
-        ]);
+        ])
+            ->assertUnprocessable()
+            ->assertJsonPath('errors.0.meta.rule', 'max')
+            ->assertJsonPath('errors.0.detail', 'The description may not be greater than 500 characters.');
 
-        $response->assertStatus(Response::HTTP_UNPROCESSABLE_ENTITY);
+        $this->postJson('/api/client/account/api-keys', [
-        $response->assertJsonPath('errors.0.meta.rule', 'max');
+                'description' => 'Foobar',
-        $response->assertJsonPath('errors.0.detail', 'The description may not be greater than 500 characters.');
+                'allowed_ips' => ['hodor', '127.0.0.1', 'hodor/24'],
+            ])
+            ->assertUnprocessable()
+            ->assertJsonPath('errors.0.detail', '"hodor" is not a valid IP address or CIDR range.')
+            ->assertJsonPath('errors.0.meta.source_field', 'allowed_ips.0')
+            ->assertJsonPath('errors.1.detail', '"hodor/24" is not a valid IP address or CIDR range.')
+            ->assertJsonPath('errors.1.meta.source_field', 'allowed_ips.2');
     }
 
     /**
@@ -156,8 +162,7 @@ class ApiKeyControllerTest extends ClientApiIntegrationTestCase
         /** @var \Pterodactyl\Models\User $user */
         $user = User::factory()->create();
         /** @var \Pterodactyl\Models\ApiKey $key */
-        $key = ApiKey::factory()->create([
+        $key = ApiKey::factory()->for($user)->create([
-            'user_id' => $user->id,
             'key_type' => ApiKey::TYPE_ACCOUNT,
         ]);
 
@@ -165,6 +170,7 @@ class ApiKeyControllerTest extends ClientApiIntegrationTestCase
         $response->assertStatus(Response::HTTP_NO_CONTENT);
 
         $this->assertDatabaseMissing('api_keys', ['id' => $key->id]);
+        $this->assertActivityFor('user:api-key.delete', $user, $user);
     }
 
     /**
@@ -184,6 +190,7 @@ class ApiKeyControllerTest extends ClientApiIntegrationTestCase
         $response->assertNotFound();
 
         $this->assertDatabaseHas('api_keys', ['id' => $key->id]);
+        Event::assertNotDispatched(ActivityLogged::class);
     }
 
     /**
@@ -197,15 +204,16 @@ class ApiKeyControllerTest extends ClientApiIntegrationTestCase
         /** @var \Pterodactyl\Models\User $user2 */
         $user2 = User::factory()->create();
         /** @var \Pterodactyl\Models\ApiKey $key */
-        $key = ApiKey::factory()->create([
+        $key = ApiKey::factory()->for($user2)->create([
-            'user_id' => $user2->id,
             'key_type' => ApiKey::TYPE_ACCOUNT,
         ]);
 
-        $response = $this->actingAs($user)->delete('/api/client/account/api-keys/' . $key->identifier);
+        $this->actingAs($user)
-        $response->assertNotFound();
+            ->deleteJson('/api/client/account/api-keys/' . $key->identifier)
+            ->assertNotFound();
 
         $this->assertDatabaseHas('api_keys', ['id' => $key->id]);
+        Event::assertNotDispatched(ActivityLogged::class);
     }
 
     /**
@@ -217,14 +225,30 @@ class ApiKeyControllerTest extends ClientApiIntegrationTestCase
         /** @var \Pterodactyl\Models\User $user */
         $user = User::factory()->create();
         /** @var \Pterodactyl\Models\ApiKey $key */
-        $key = ApiKey::factory()->create([
+        $key = ApiKey::factory()->for($user)->create([
-            'user_id' => $user->id,
             'key_type' => ApiKey::TYPE_APPLICATION,
         ]);
 
-        $response = $this->actingAs($user)->delete('/api/client/account/api-keys/' . $key->identifier);
+        $this->actingAs($user)
-        $response->assertNotFound();
+            ->deleteJson('/api/client/account/api-keys/' . $key->identifier)
+            ->assertNotFound();
 
         $this->assertDatabaseHas('api_keys', ['id' => $key->id]);
     }
+
+    /**
+     * Provides some different IP address combinations that can be used when
+     * testing that we accept the expected IP values.
+     */
+    public function validIPAddressDataProvider(): array
+    {
+        return [
+            [[]],
+            [['127.0.0.1']],
+            [['127.0.0.1', '::1']],
+            [['::ffff:7f00:1']],
+            [['127.0.0.1', '192.168.1.100', '192.168.10.10/28']],
+            [['127.0.0.1/32', '192.168.100.100/27', '::1', '::1/128']],
+        ];
+    }
 }

tests/Integration/IntegrationTestCase.php

@@ -4,12 +4,16 @@ namespace Pterodactyl\Tests\Integration;
 
 use Carbon\CarbonImmutable;
 use Pterodactyl\Tests\TestCase;
+use Illuminate\Support\Facades\Event;
+use Pterodactyl\Events\ActivityLogged;
+use Pterodactyl\Tests\Assertions\AssertsActivityLogged;
 use Pterodactyl\Tests\Traits\Integration\CreatesTestModels;
 use Pterodactyl\Transformers\Api\Application\BaseTransformer;
 
 abstract class IntegrationTestCase extends TestCase
 {
     use CreatesTestModels;
+    use AssertsActivityLogged;
 
     protected array $connectionsToTransact = ['mysql'];
 
@@ -17,6 +21,13 @@ abstract class IntegrationTestCase extends TestCase
         'Accept' => 'application/json',
     ];
 
+    public function setUp(): void
+    {
+        parent::setUp();
+
+        Event::fake(ActivityLogged::class);
+    }
+
     /**
      * Return an ISO-8601 formatted timestamp to use in the API response.
      */