From d4bf6bd46a4e50673891ba4a7951cd292811f8be Mon Sep 17 00:00:00 2001
From: DaneEveritt <dane@daneeveritt.com>
Date: Sun, 15 May 2022 17:30:57 -0400
Subject: [PATCH] Add test coverage and fix permissions mistake
---
.../Remote/SftpAuthenticationController.php | 4 +-
.../Client/ClientApiIntegrationTestCase.php | 30 ---
.../SftpAuthenticationControllerTest.php | 250 ++++++++++++++++++
.../Traits/Integration/CreatesTestModels.php | 30 +++
4 files changed, 282 insertions(+), 32 deletions(-)
create mode 100644 tests/Integration/Api/Remote/SftpAuthenticationControllerTest.php
diff --git a/app/Http/Controllers/Api/Remote/SftpAuthenticationController.php b/app/Http/Controllers/Api/Remote/SftpAuthenticationController.php
index 7d426150..8a687490 100644
--- a/app/Http/Controllers/Api/Remote/SftpAuthenticationController.php
+++ b/app/Http/Controllers/Api/Remote/SftpAuthenticationController.php
@@ -62,7 +62,7 @@ class SftpAuthenticationController extends Controller
}
if (!$key || !$user->sshKeys()->where('fingerprint', $key->getFingerprint('sha256'))->exists()) {
- $this->reject($request, false);
+ $this->reject($request, is_null($key));
}
}
@@ -70,7 +70,7 @@ class SftpAuthenticationController extends Controller
return new JsonResponse([
'server' => $server->uuid,
- 'permissions' => $permissions ?? ['*'],
+ 'permissions' => $this->permissions->handle($server, $user),
]);
}
diff --git a/tests/Integration/Api/Client/ClientApiIntegrationTestCase.php b/tests/Integration/Api/Client/ClientApiIntegrationTestCase.php
index 35e8d271..03b80eba 100644
--- a/tests/Integration/Api/Client/ClientApiIntegrationTestCase.php
+++ b/tests/Integration/Api/Client/ClientApiIntegrationTestCase.php
@@ -9,7 +9,6 @@ use Pterodactyl\Models\User;
use InvalidArgumentException;
use Pterodactyl\Models\Backup;
use Pterodactyl\Models\Server;
-use Pterodactyl\Models\Subuser;
use Pterodactyl\Models\Database;
use Pterodactyl\Models\Location;
use Pterodactyl\Models\Schedule;
@@ -88,35 +87,6 @@ abstract class ClientApiIntegrationTestCase extends IntegrationTestCase
return $link . ($append ? '/' . ltrim($append, '/') : '');
}
- /**
- * Generates a user and a server for that user. If an array of permissions is passed it
- * is assumed that the user is actually a subuser of the server.
- *
- * @param string[] $permissions
- *
- * @return array{\Pterodactyl\Models\User, \Pterodactyl\Models\Server}
- */
- protected function generateTestAccount(array $permissions = []): array
- {
- /** @var \Pterodactyl\Models\User $user */
- $user = User::factory()->create();
-
- if (empty($permissions)) {
- return [$user, $this->createServerModel(['user_id' => $user->id])];
- }
-
- /** @var \Pterodactyl\Models\Server $server */
- $server = $this->createServerModel();
-
- Subuser::query()->create([
- 'user_id' => $user->id,
- 'server_id' => $server->id,
- 'permissions' => $permissions,
- ]);
-
- return [$user, $server];
- }
-
/**
* Asserts that the data passed through matches the output of the data from the transformer. This
* will remove the "relationships" key when performing the comparison.
diff --git a/tests/Integration/Api/Remote/SftpAuthenticationControllerTest.php b/tests/Integration/Api/Remote/SftpAuthenticationControllerTest.php
new file mode 100644
index 00000000..4ef485db
--- /dev/null
+++ b/tests/Integration/Api/Remote/SftpAuthenticationControllerTest.php
@@ -0,0 +1,250 @@
+<?php
+
+namespace Pterodactyl\Tests\Integration\Api\Remote;
+
+use Pterodactyl\Models\Node;
+use Pterodactyl\Models\User;
+use Pterodactyl\Models\Server;
+use Pterodactyl\Models\Permission;
+use Pterodactyl\Models\UserSSHKey;
+use phpseclib3\Crypt\EC\PrivateKey;
+use Pterodactyl\Tests\Integration\IntegrationTestCase;
+
+class SftpAuthenticationControllerTest extends IntegrationTestCase
+{
+ protected User $user;
+
+ protected Server $server;
+
+ /**
+ * Sets up the tests.
+ */
+ public function setUp(): void
+ {
+ parent::setUp();
+
+ [$user, $server] = $this->generateTestAccount();
+
+ $user->update(['password' => password_hash('foobar', PASSWORD_DEFAULT)]);
+
+ $this->user = $user;
+ $this->server = $server;
+
+ $this->setAuthorization();
+ }
+
+ /**
+ * Test that a public key is validated correctly.
+ */
+ public function testPublicKeyIsValidatedCorrectly()
+ {
+ $key = UserSSHKey::factory()->for($this->user)->create();
+
+ $this->postJson('/api/remote/sftp/auth', [])
+ ->assertUnprocessable()
+ ->assertJsonPath('errors.0.meta.source_field', 'username')
+ ->assertJsonPath('errors.0.meta.rule', 'required')
+ ->assertJsonPath('errors.1.meta.source_field', 'password')
+ ->assertJsonPath('errors.1.meta.rule', 'required');
+
+ $data = [
+ 'type' => 'public_key',
+ 'username' => $this->getUsername(),
+ 'password' => $key->public_key,
+ ];
+
+ $this->postJson('/api/remote/sftp/auth', $data)
+ ->assertOk()
+ ->assertJsonPath('server', $this->server->uuid)
+ ->assertJsonPath('permissions', ['*']);
+
+ $key->delete();
+ $this->postJson('/api/remote/sftp/auth', $data)->assertForbidden();
+ $this->postJson('/api/remote/sftp/auth', array_merge($data, ['type' => null]))->assertForbidden();
+ }
+
+ /**
+ * Test that an account password is validated correctly.
+ */
+ public function testPasswordIsValidatedCorrectly()
+ {
+ $this->postJson('/api/remote/sftp/auth', [
+ 'username' => $this->getUsername(),
+ 'password' => '',
+ ])
+ ->assertUnprocessable()
+ ->assertJsonPath('errors.0.meta.source_field', 'password')
+ ->assertJsonPath('errors.0.meta.rule', 'required');
+
+ $this->postJson('/api/remote/sftp/auth', [
+ 'username' => $this->getUsername(),
+ 'password' => 'wrong password',
+ ])
+ ->assertForbidden();
+
+ $this->user->update(['password' => password_hash('foobar', PASSWORD_DEFAULT)]);
+
+ $this->postJson('/api/remote/sftp/auth', [
+ 'username' => $this->getUsername(),
+ 'password' => 'foobar',
+ ])
+ ->assertOk();
+ }
+
+ /**
+ * Test that providing an invalid key and/or invalid username triggers the throttle on
+ * the endpoint.
+ *
+ * @dataProvider authorizationTypeDataProvider
+ */
+ public function testUserIsThrottledIfInvalidCredentialsAreProvided(string $type)
+ {
+ for ($i = 0; $i <= 10; ++$i) {
+ $this->postJson('/api/remote/sftp/auth', [
+ 'type' => 'public_key',
+ 'username' => $i % 2 === 0 ? $this->user->username : $this->getUsername(),
+ 'password' => 'invalid key',
+ ])
+ ->assertStatus($i === 10 ? 429 : 403);
+ }
+ }
+
+ /**
+ * Test that the user is not throttled so long as a valid public key is provided, even
+ * if it doesn't actually exist in the database for the user.
+ */
+ public function testUserIsNotThrottledIfNoPublicKeyMatches()
+ {
+ for ($i = 0; $i <= 10; ++$i) {
+ $this->postJson('/api/remote/sftp/auth', [
+ 'type' => 'public_key',
+ 'username' => $this->getUsername(),
+ 'password' => PrivateKey::createKey('Ed25519')->getPublicKey()->toString('OpenSSH'),
+ ])
+ ->assertForbidden();
+ }
+ }
+
+ /**
+ * Test that a request is rejected if the credentials are valid but the username indicates
+ * a server on a different node.
+ *
+ * @dataProvider authorizationTypeDataProvider
+ */
+ public function testRequestIsRejectedIfServerBelongsToDifferentNode(string $type)
+ {
+ $node2 = $this->createServerModel()->node;
+
+ $this->setAuthorization($node2);
+
+ $password = $type === 'public_key'
+ ? UserSSHKey::factory()->for($this->user)->create()->public_key
+ : 'foobar';
+
+ $this->postJson('/api/remote/sftp/auth', [
+ 'type' => 'public_key',
+ 'username' => $this->getUsername(),
+ 'password' => $password,
+ ])
+ ->assertForbidden();
+ }
+
+ public function testRequestIsDeniedIfUserLacksSftpPermission()
+ {
+ [$user, $server] = $this->generateTestAccount([Permission::ACTION_FILE_READ]);
+
+ $user->update(['password' => password_hash('foobar', PASSWORD_DEFAULT)]);
+
+ $this->setAuthorization($server->node);
+
+ $this->postJson('/api/remote/sftp/auth', [
+ 'username' => $user->username . '.' . $server->uuidShort,
+ 'password' => 'foobar',
+ ])
+ ->assertForbidden()
+ ->assertJsonPath('errors.0.detail', 'You do not have permission to access SFTP for this server.');
+ }
+
+ /**
+ * @dataProvider serverStateDataProvider
+ */
+ public function testInvalidServerStateReturnsConflictError(string $status)
+ {
+ $this->server->update(['status' => $status]);
+
+ $this->postJson('/api/remote/sftp/auth', ['username' => $this->getUsername(), 'password' => 'foobar'])
+ ->assertStatus(409);
+ }
+
+ /**
+ * Test that permissions are returned for the user account correctly.
+ */
+ public function testUserPermissionsAreReturnedCorrectly()
+ {
+ [$user, $server] = $this->generateTestAccount([Permission::ACTION_FILE_READ, Permission::ACTION_FILE_SFTP]);
+
+ $user->update(['password' => password_hash('foobar', PASSWORD_DEFAULT)]);
+
+ $this->setAuthorization($server->node);
+
+ $data = [
+ 'username' => $user->username . '.' . $server->uuidShort,
+ 'password' => 'foobar',
+ ];
+
+ $this->postJson('/api/remote/sftp/auth', $data)
+ ->assertOk()
+ ->assertJsonPath('permissions', [Permission::ACTION_FILE_READ, Permission::ACTION_FILE_SFTP]);
+
+ $user->update(['root_admin' => true]);
+
+ $this->postJson('/api/remote/sftp/auth', $data)
+ ->assertOk()
+ ->assertJsonPath('permissions.0', '*');
+
+ $this->setAuthorization();
+ $data['username'] = $user->username . '.' . $this->server->uuidShort;
+
+ $this->post('/api/remote/sftp/auth', $data)
+ ->assertOk()
+ ->assertJsonPath('permissions.0', '*');
+
+ $user->update(['root_admin' => false]);
+ $this->post('/api/remote/sftp/auth', $data)->assertForbidden();
+ }
+
+ public function authorizationTypeDataProvider(): array
+ {
+ return [
+ 'password auth' => ['password'],
+ 'public key auth' => ['public_key'],
+ ];
+ }
+
+ public function serverStateDataProvider(): array
+ {
+ return [
+ 'installing' => [Server::STATUS_INSTALLING],
+ 'suspended' => [Server::STATUS_SUSPENDED],
+ 'restoring a backup' => [Server::STATUS_RESTORING_BACKUP],
+ ];
+ }
+
+ /**
+ * Returns the username for connecting to SFTP.
+ */
+ protected function getUsername(bool $long = false): string
+ {
+ return $this->user->username . '.' . ($long ? $this->server->uuid : $this->server->uuidShort);
+ }
+
+ /**
+ * Sets the authorization header for the rest of the test.
+ */
+ protected function setAuthorization(Node $node = null): void
+ {
+ $node = $node ?? $this->server->node;
+
+ $this->withHeader('Authorization', 'Bearer ' . $node->daemon_token_id . '.' . decrypt($node->daemon_token));
+ }
+}
diff --git a/tests/Traits/Integration/CreatesTestModels.php b/tests/Traits/Integration/CreatesTestModels.php
index 0885ccc5..51e4b76a 100644
--- a/tests/Traits/Integration/CreatesTestModels.php
+++ b/tests/Traits/Integration/CreatesTestModels.php
@@ -7,6 +7,7 @@ use Pterodactyl\Models\Egg;
use Pterodactyl\Models\Node;
use Pterodactyl\Models\User;
use Pterodactyl\Models\Server;
+use Pterodactyl\Models\Subuser;
use Pterodactyl\Models\Location;
use Pterodactyl\Models\Allocation;
@@ -76,6 +77,35 @@ trait CreatesTestModels
]);
}
+ /**
+ * Generates a user and a server for that user. If an array of permissions is passed it
+ * is assumed that the user is actually a subuser of the server.
+ *
+ * @param string[] $permissions
+ *
+ * @return array{\Pterodactyl\Models\User, \Pterodactyl\Models\Server}
+ */
+ public function generateTestAccount(array $permissions = []): array
+ {
+ /** @var \Pterodactyl\Models\User $user */
+ $user = User::factory()->create();
+
+ if (empty($permissions)) {
+ return [$user, $this->createServerModel(['user_id' => $user->id])];
+ }
+
+ /** @var \Pterodactyl\Models\Server $server */
+ $server = $this->createServerModel();
+
+ Subuser::query()->create([
+ 'user_id' => $user->id,
+ 'server_id' => $server->id,
+ 'permissions' => $permissions,
+ ]);
+
+ return [$user, $server];
+ }
+
/**
* Clones a given egg allowing us to make modifications that don't affect other
* tests that rely on the egg existing in the correct state.