Skip to content

Commit d4bf6bd

Browse files
DaneEverittDaneEveritt
committed
Add test coverage and fix permissions mistake
1 parent 8483db7 commit d4bf6bd

File tree

4 files changed

+282
-32
lines changed

4 files changed

+282
-32
lines changed

‎app/Http/Controllers/Api/Remote/SftpAuthenticationController.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,15 +62,15 @@ public function __invoke(SftpAuthenticationFormRequest $request): JsonResponse
6262
}
6363

6464
if (!$key || !$user->sshKeys()->where('fingerprint', $key->getFingerprint('sha256'))->exists()) {
65-
$this->reject($request, false);
65+
$this->reject($request, is_null($key));
6666
}
6767
}
6868

6969
$this->validateSftpAccess($user, $server);
7070

7171
return new JsonResponse([
7272
'server' => $server->uuid,
73-
'permissions' => $permissions ?? ['*'],
73+
'permissions' => $this->permissions->handle($server, $user),
7474
]);
7575
}
7676

‎tests/Integration/Api/Client/ClientApiIntegrationTestCase.php‎

Lines changed: 0 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@
99
use InvalidArgumentException;
1010
use Pterodactyl\Models\Backup;
1111
use Pterodactyl\Models\Server;
12-
use Pterodactyl\Models\Subuser;
1312
use Pterodactyl\Models\Database;
1413
use Pterodactyl\Models\Location;
1514
use Pterodactyl\Models\Schedule;
@@ -88,35 +87,6 @@ protected function link($model, $append = null): string
8887
return $link . ($append ? '/' . ltrim($append, '/') : '');
8988
}
9089

91-
/**
92-
* Generates a user and a server for that user. If an array of permissions is passed it
93-
* is assumed that the user is actually a subuser of the server.
94-
*
95-
* @param string[] $permissions
96-
*
97-
* @return array{\Pterodactyl\Models\User, \Pterodactyl\Models\Server}
98-
*/
99-
protected function generateTestAccount(array $permissions = []): array
100-
{
101-
/** @var \Pterodactyl\Models\User $user */
102-
$user = User::factory()->create();
103-
104-
if (empty($permissions)) {
105-
return [$user, $this->createServerModel(['user_id' => $user->id])];
106-
}
107-
108-
/** @var \Pterodactyl\Models\Server $server */
109-
$server = $this->createServerModel();
110-
111-
Subuser::query()->create([
112-
'user_id' => $user->id,
113-
'server_id' => $server->id,
114-
'permissions' => $permissions,
115-
]);
116-
117-
return [$user, $server];
118-
}
119-
12090
/**
12191
* Asserts that the data passed through matches the output of the data from the transformer. This
12292
* will remove the "relationships" key when performing the comparison.

‎tests/Integration/Api/Remote/SftpAuthenticationControllerTest.php‎

Lines changed: 250 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,250 @@
1+
<?php
2+
3+
namespace Pterodactyl\Tests\Integration\Api\Remote;
4+
5+
use Pterodactyl\Models\Node;
6+
use Pterodactyl\Models\User;
7+
use Pterodactyl\Models\Server;
8+
use Pterodactyl\Models\Permission;
9+
use Pterodactyl\Models\UserSSHKey;
10+
use phpseclib3\Crypt\EC\PrivateKey;
11+
use Pterodactyl\Tests\Integration\IntegrationTestCase;
12+
13+
class SftpAuthenticationControllerTest extends IntegrationTestCase
14+
{
15+
protected User $user;
16+
17+
protected Server $server;
18+
19+
/**
20+
* Sets up the tests.
21+
*/
22+
public function setUp(): void
23+
{
24+
parent::setUp();
25+
26+
[$user, $server] = $this->generateTestAccount();
27+
28+
$user->update(['password' => password_hash('foobar', PASSWORD_DEFAULT)]);
29+
30+
$this->user = $user;
31+
$this->server = $server;
32+
33+
$this->setAuthorization();
34+
}
35+
36+
/**
37+
* Test that a public key is validated correctly.
38+
*/
39+
public function testPublicKeyIsValidatedCorrectly()
40+
{
41+
$key = UserSSHKey::factory()->for($this->user)->create();
42+
43+
$this->postJson('/api/remote/sftp/auth', [])
44+
->assertUnprocessable()
45+
->assertJsonPath('errors.0.meta.source_field', 'username')
46+
->assertJsonPath('errors.0.meta.rule', 'required')
47+
->assertJsonPath('errors.1.meta.source_field', 'password')
48+
->assertJsonPath('errors.1.meta.rule', 'required');
49+
50+
$data = [
51+
'type' => 'public_key',
52+
'username' => $this->getUsername(),
53+
'password' => $key->public_key,
54+
];
55+
56+
$this->postJson('/api/remote/sftp/auth', $data)
57+
->assertOk()
58+
->assertJsonPath('server', $this->server->uuid)
59+
->assertJsonPath('permissions', ['*']);
60+
61+
$key->delete();
62+
$this->postJson('/api/remote/sftp/auth', $data)->assertForbidden();
63+
$this->postJson('/api/remote/sftp/auth', array_merge($data, ['type' => null]))->assertForbidden();
64+
}
65+
66+
/**
67+
* Test that an account password is validated correctly.
68+
*/
69+
public function testPasswordIsValidatedCorrectly()
70+
{
71+
$this->postJson('/api/remote/sftp/auth', [
72+
'username' => $this->getUsername(),
73+
'password' => '',
74+
])
75+
->assertUnprocessable()
76+
->assertJsonPath('errors.0.meta.source_field', 'password')
77+
->assertJsonPath('errors.0.meta.rule', 'required');
78+
79+
$this->postJson('/api/remote/sftp/auth', [
80+
'username' => $this->getUsername(),
81+
'password' => 'wrong password',
82+
])
83+
->assertForbidden();
84+
85+
$this->user->update(['password' => password_hash('foobar', PASSWORD_DEFAULT)]);
86+
87+
$this->postJson('/api/remote/sftp/auth', [
88+
'username' => $this->getUsername(),
89+
'password' => 'foobar',
90+
])
91+
->assertOk();
92+
}
93+
94+
/**
95+
* Test that providing an invalid key and/or invalid username triggers the throttle on
96+
* the endpoint.
97+
*
98+
* @dataProvider authorizationTypeDataProvider
99+
*/
100+
public function testUserIsThrottledIfInvalidCredentialsAreProvided(string $type)
101+
{
102+
for ($i = 0; $i <= 10; ++$i) {
103+
$this->postJson('/api/remote/sftp/auth', [
104+
'type' => 'public_key',
105+
'username' => $i % 2 === 0 ? $this->user->username : $this->getUsername(),
106+
'password' => 'invalid key',
107+
])
108+
->assertStatus($i === 10 ? 429 : 403);
109+
}
110+
}
111+
112+
/**
113+
* Test that the user is not throttled so long as a valid public key is provided, even
114+
* if it doesn't actually exist in the database for the user.
115+
*/
116+
public function testUserIsNotThrottledIfNoPublicKeyMatches()
117+
{
118+
for ($i = 0; $i <= 10; ++$i) {
119+
$this->postJson('/api/remote/sftp/auth', [
120+
'type' => 'public_key',
121+
'username' => $this->getUsername(),
122+
'password' => PrivateKey::createKey('Ed25519')->getPublicKey()->toString('OpenSSH'),
123+
])
124+
->assertForbidden();
125+
}
126+
}
127+
128+
/**
129+
* Test that a request is rejected if the credentials are valid but the username indicates
130+
* a server on a different node.
131+
*
132+
* @dataProvider authorizationTypeDataProvider
133+
*/
134+
public function testRequestIsRejectedIfServerBelongsToDifferentNode(string $type)
135+
{
136+
$node2 = $this->createServerModel()->node;
137+
138+
$this->setAuthorization($node2);
139+
140+
$password = $type === 'public_key'
141+
? UserSSHKey::factory()->for($this->user)->create()->public_key
142+
: 'foobar';
143+
144+
$this->postJson('/api/remote/sftp/auth', [
145+
'type' => 'public_key',
146+
'username' => $this->getUsername(),
147+
'password' => $password,
148+
])
149+
->assertForbidden();
150+
}
151+
152+
public function testRequestIsDeniedIfUserLacksSftpPermission()
153+
{
154+
[$user, $server] = $this->generateTestAccount([Permission::ACTION_FILE_READ]);
155+
156+
$user->update(['password' => password_hash('foobar', PASSWORD_DEFAULT)]);
157+
158+
$this->setAuthorization($server->node);
159+
160+
$this->postJson('/api/remote/sftp/auth', [
161+
'username' => $user->username . '.' . $server->uuidShort,
162+
'password' => 'foobar',
163+
])
164+
->assertForbidden()
165+
->assertJsonPath('errors.0.detail', 'You do not have permission to access SFTP for this server.');
166+
}
167+
168+
/**
169+
* @dataProvider serverStateDataProvider
170+
*/
171+
public function testInvalidServerStateReturnsConflictError(string $status)
172+
{
173+
$this->server->update(['status' => $status]);
174+
175+
$this->postJson('/api/remote/sftp/auth', ['username' => $this->getUsername(), 'password' => 'foobar'])
176+
->assertStatus(409);
177+
}
178+
179+
/**
180+
* Test that permissions are returned for the user account correctly.
181+
*/
182+
public function testUserPermissionsAreReturnedCorrectly()
183+
{
184+
[$user, $server] = $this->generateTestAccount([Permission::ACTION_FILE_READ, Permission::ACTION_FILE_SFTP]);
185+
186+
$user->update(['password' => password_hash('foobar', PASSWORD_DEFAULT)]);
187+
188+
$this->setAuthorization($server->node);
189+
190+
$data = [
191+
'username' => $user->username . '.' . $server->uuidShort,
192+
'password' => 'foobar',
193+
];
194+
195+
$this->postJson('/api/remote/sftp/auth', $data)
196+
->assertOk()
197+
->assertJsonPath('permissions', [Permission::ACTION_FILE_READ, Permission::ACTION_FILE_SFTP]);
198+
199+
$user->update(['root_admin' => true]);
200+
201+
$this->postJson('/api/remote/sftp/auth', $data)
202+
->assertOk()
203+
->assertJsonPath('permissions.0', '*');
204+
205+
$this->setAuthorization();
206+
$data['username'] = $user->username . '.' . $this->server->uuidShort;
207+
208+
$this->post('/api/remote/sftp/auth', $data)
209+
->assertOk()
210+
->assertJsonPath('permissions.0', '*');
211+
212+
$user->update(['root_admin' => false]);
213+
$this->post('/api/remote/sftp/auth', $data)->assertForbidden();
214+
}
215+
216+
public function authorizationTypeDataProvider(): array
217+
{
218+
return [
219+
'password auth' => ['password'],
220+
'public key auth' => ['public_key'],
221+
];
222+
}
223+
224+
public function serverStateDataProvider(): array
225+
{
226+
return [
227+
'installing' => [Server::STATUS_INSTALLING],
228+
'suspended' => [Server::STATUS_SUSPENDED],
229+
'restoring a backup' => [Server::STATUS_RESTORING_BACKUP],
230+
];
231+
}
232+
233+
/**
234+
* Returns the username for connecting to SFTP.
235+
*/
236+
protected function getUsername(bool $long = false): string
237+
{
238+
return $this->user->username . '.' . ($long ? $this->server->uuid : $this->server->uuidShort);
239+
}
240+
241+
/**
242+
* Sets the authorization header for the rest of the test.
243+
*/
244+
protected function setAuthorization(Node $node = null): void
245+
{
246+
$node = $node ?? $this->server->node;
247+
248+
$this->withHeader('Authorization', 'Bearer ' . $node->daemon_token_id . '.' . decrypt($node->daemon_token));
249+
}
250+
}

‎tests/Traits/Integration/CreatesTestModels.php‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
use Pterodactyl\Models\Node;
88
use Pterodactyl\Models\User;
99
use Pterodactyl\Models\Server;
10+
use Pterodactyl\Models\Subuser;
1011
use Pterodactyl\Models\Location;
1112
use Pterodactyl\Models\Allocation;
1213

@@ -76,6 +77,35 @@ public function createServerModel(array $attributes = [])
7677
]);
7778
}
7879

80+
/**
81+
* Generates a user and a server for that user. If an array of permissions is passed it
82+
* is assumed that the user is actually a subuser of the server.
83+
*
84+
* @param string[] $permissions
85+
*
86+
* @return array{\Pterodactyl\Models\User, \Pterodactyl\Models\Server}
87+
*/
88+
public function generateTestAccount(array $permissions = []): array
89+
{
90+
/** @var \Pterodactyl\Models\User $user */
91+
$user = User::factory()->create();
92+
93+
if (empty($permissions)) {
94+
return [$user, $this->createServerModel(['user_id' => $user->id])];
95+
}
96+
97+
/** @var \Pterodactyl\Models\Server $server */
98+
$server = $this->createServerModel();
99+
100+
Subuser::query()->create([
101+
'user_id' => $user->id,
102+
'server_id' => $server->id,
103+
'permissions' => $permissions,
104+
]);
105+
106+
return [$user, $server];
107+
}
108+
79109
/**
80110
* Clones a given egg allowing us to make modifications that don't affect other
81111
* tests that rely on the egg existing in the correct state.

0 commit comments

Comments
 (0)