Fix up database creation and handling code for servers; ref pterodactyl#2447

DaneEveritt · DaneEveritt · commit 8697185900bb · 2020-10-11T11:59:46.000-07:00
diff --git a/app/Contracts/Repository/DatabaseRepositoryInterface.php b/app/Contracts/Repository/DatabaseRepositoryInterface.php
@@ -42,18 +42,6 @@ public function getDatabasesForServer(int $server): Collection;
      */
     public function getDatabasesForHost(int $host, int $count = 25): LengthAwarePaginator;
 
-    /**
-     * Create a new database if it does not already exist on the host with
-     * the provided details.
-     *
-     * @param array $data
-     * @return \Pterodactyl\Models\Database
-     *
-     * @throws \Pterodactyl\Exceptions\Model\DataValidationException
-     * @throws \Pterodactyl\Exceptions\Repository\DuplicateDatabaseNameException
-     */
-    public function createIfNotExists(array $data): Database;
-
     /**
      * Create a new database on a given connection.
      *
diff --git a/app/Http/Controllers/Admin/ServersController.php b/app/Http/Controllers/Admin/ServersController.php
@@ -362,7 +362,7 @@ public function saveStartup(Request $request, Server $server)
     public function newDatabase(StoreServerDatabaseRequest $request, Server $server)
     {
         $this->databaseManagementService->create($server, [
-            'database' => $request->input('database'),
+            'database' => DatabaseManagementService::generateUniqueDatabaseName($request->input('database'), $server->id),
             'remote' => $request->input('remote'),
             'database_host_id' => $request->input('database_host_id'),
             'max_connections' => $request->input('max_connections'),
diff --git a/app/Http/Controllers/Api/Application/Servers/DatabaseController.php b/app/Http/Controllers/Api/Application/Servers/DatabaseController.php
@@ -110,7 +110,9 @@ public function resetPassword(ServerDatabaseWriteRequest $request, Server $serve
      */
     public function store(StoreServerDatabaseRequest $request, Server $server): JsonResponse
     {
-        $database = $this->databaseManagementService->create($server, $request->validated());
+        $database = $this->databaseManagementService->create($server, array_merge($request->validated(), [
+            'database' => $request->databaseName(),
+        ]));
 
         return $this->fractal->item($database)
             ->transformWith($this->getTransformer(ServerDatabaseTransformer::class))
diff --git a/app/Http/Requests/Api/Application/Servers/Databases/StoreServerDatabaseRequest.php b/app/Http/Requests/Api/Application/Servers/Databases/StoreServerDatabaseRequest.php
@@ -2,9 +2,12 @@
 
 namespace Pterodactyl\Http\Requests\Api\Application\Servers\Databases;
 
+use Webmozart\Assert\Assert;
+use Pterodactyl\Models\Server;
 use Illuminate\Validation\Rule;
 use Illuminate\Database\Query\Builder;
 use Pterodactyl\Services\Acl\Api\AdminAcl;
+use Pterodactyl\Services\Databases\DatabaseManagementService;
 use Pterodactyl\Http\Requests\Api\Application\ApplicationApiRequest;
 
 class StoreServerDatabaseRequest extends ApplicationApiRequest
@@ -26,14 +29,16 @@ class StoreServerDatabaseRequest extends ApplicationApiRequest
      */
     public function rules(): array
     {
+        $server = $this->route()->parameter('server');
+
         return [
             'database' => [
                 'required',
-                'string',
+                'alpha_dash',
                 'min:1',
-                'max:24',
-                Rule::unique('databases')->where(function (Builder $query) {
-                    $query->where('database_host_id', $this->input('host') ?? 0);
+                'max:48',
+                Rule::unique('databases')->where(function (Builder $query) use ($server) {
+                    $query->where('server_id', $server->id)->where('database', $this->databaseName());
                 }),
             ],
             'remote' => 'required|string|regex:/^[0-9%.]{1,15}$/',
@@ -68,4 +73,18 @@ public function attributes()
             'database' => 'Database Name',
         ];
     }
+
+    /**
+     * Returns the database name in the expected format.
+     *
+     * @return string
+     */
+    public function databaseName(): string
+    {
+        $server = $this->route()->parameter('server');
+
+        Assert::isInstanceOf($server, Server::class);
+
+        return DatabaseManagementService::generateUniqueDatabaseName($this->input('database'), $server->id);
+    }
 }
diff --git a/app/Http/Requests/Api/Client/Servers/Databases/StoreDatabaseRequest.php b/app/Http/Requests/Api/Client/Servers/Databases/StoreDatabaseRequest.php
@@ -9,6 +9,7 @@
 use Illuminate\Database\Query\Builder;
 use Pterodactyl\Contracts\Http\ClientPermissionsRequest;
 use Pterodactyl\Http\Requests\Api\Client\ClientApiRequest;
+use Pterodactyl\Services\Databases\DatabaseManagementService;
 
 class StoreDatabaseRequest extends ClientApiRequest implements ClientPermissionsRequest
 {
@@ -33,19 +34,23 @@ public function rules(): array
             'database' => [
                 'required',
                 'alpha_dash',
-                'min:3',
+                'min:1',
                 'max:48',
                 // Yes, I am aware that you could have the same database name across two unique hosts. However,
                 // I don't really care about that for this validation. We just want to make sure it is unique to
                 // the server itself. No need for complexity.
-                Rule::unique('databases', 'database')->where(function (Builder $query) use ($server) {
-                    $query->where('server_id', $server->id);
+                Rule::unique('databases')->where(function (Builder $query) use ($server) {
+                    $query->where('server_id', $server->id)
+                        ->where('database', DatabaseManagementService::generateUniqueDatabaseName($this->input('database'), $server->id));
                 }),
             ],
             'remote' => 'required|string|regex:/^[0-9%.]{1,15}$/',
         ];
     }
 
+    /**
+     * @return array
+     */
     public function messages()
     {
         return [
diff --git a/app/Repositories/Eloquent/DatabaseRepository.php b/app/Repositories/Eloquent/DatabaseRepository.php
@@ -93,31 +93,6 @@ public function getDatabasesForHost(int $host, int $count = 25): LengthAwarePagi
             ->paginate($count, $this->getColumns());
     }
 
-    /**
-     * Create a new database if it does not already exist on the host with
-     * the provided details.
-     *
-     * @param array $data
-     * @return \Pterodactyl\Models\Database
-     *
-     * @throws \Pterodactyl\Exceptions\Model\DataValidationException
-     * @throws \Pterodactyl\Exceptions\Repository\DuplicateDatabaseNameException
-     */
-    public function createIfNotExists(array $data): Database
-    {
-        $count = $this->getBuilder()->where([
-            ['server_id', '=', array_get($data, 'server_id')],
-            ['database_host_id', '=', array_get($data, 'database_host_id')],
-            ['database', '=', array_get($data, 'database')],
-        ])->count();
-
-        if ($count > 0) {
-            throw new DuplicateDatabaseNameException('A database with those details already exists for the specified server.');
-        }
-
-        return $this->create($data);
-    }
-
     /**
      * Create a new database on a given connection.
      *
diff --git a/app/Services/Databases/DatabaseManagementService.php b/app/Services/Databases/DatabaseManagementService.php
@@ -3,18 +3,29 @@
 namespace Pterodactyl\Services\Databases;
 
 use Exception;
+use InvalidArgumentException;
 use Pterodactyl\Models\Server;
 use Pterodactyl\Models\Database;
 use Pterodactyl\Helpers\Utilities;
 use Illuminate\Database\ConnectionInterface;
+use Symfony\Component\VarDumper\Cloner\Data;
 use Illuminate\Contracts\Encryption\Encrypter;
 use Pterodactyl\Extensions\DynamicDatabaseConnection;
-use Pterodactyl\Contracts\Repository\DatabaseRepositoryInterface;
+use Pterodactyl\Repositories\Eloquent\DatabaseRepository;
+use Pterodactyl\Exceptions\Repository\DuplicateDatabaseNameException;
 use Pterodactyl\Exceptions\Service\Database\TooManyDatabasesException;
 use Pterodactyl\Exceptions\Service\Database\DatabaseClientFeatureNotEnabledException;
 
 class DatabaseManagementService
 {
+    /**
+     * The regex used to validate that the database name passed through to the function is
+     * in the expected format.
+     *
+     * @see \Pterodactyl\Services\Databases\DatabaseManagementService::generateUniqueDatabaseName()
+     */
+    private const MATCH_NAME_REGEX = '/^(s[\d]+_)(.*)$/';
+
     /**
      * @var \Illuminate\Database\ConnectionInterface
      */
@@ -31,7 +42,7 @@ class DatabaseManagementService
     private $encrypter;
 
     /**
-     * @var \Pterodactyl\Contracts\Repository\DatabaseRepositoryInterface
+     * @var \Pterodactyl\Repositories\Eloquent\DatabaseRepository
      */
     private $repository;
 
@@ -50,13 +61,13 @@ class DatabaseManagementService
      *
      * @param \Illuminate\Database\ConnectionInterface $connection
      * @param \Pterodactyl\Extensions\DynamicDatabaseConnection $dynamic
-     * @param \Pterodactyl\Contracts\Repository\DatabaseRepositoryInterface $repository
+     * @param \Pterodactyl\Repositories\Eloquent\DatabaseRepository $repository
      * @param \Illuminate\Contracts\Encryption\Encrypter $encrypter
      */
     public function __construct(
         ConnectionInterface $connection,
         DynamicDatabaseConnection $dynamic,
-        DatabaseRepositoryInterface $repository,
+        DatabaseRepository $repository,
         Encrypter $encrypter
     ) {
         $this->connection = $connection;
@@ -65,6 +76,21 @@ public function __construct(
         $this->repository = $repository;
     }
 
+    /**
+     * Generates a unique database name for the given server. This name should be passed through when
+     * calling this handle function for this service, otherwise the database will be created with
+     * whatever name is provided.
+     *
+     * @param string $name
+     * @param int $serverId
+     * @return string
+     */
+    public static function generateUniqueDatabaseName(string $name, int $serverId): string
+    {
+        // Max of 48 characters, including the s123_ that we append to the front.
+        return sprintf('s%d_%s', $serverId, substr($name, 0, 48 - strlen("s{$serverId}_")));
+    }
+
     /**
      * Set wether or not this class should validate that the server has enough slots
      * left before creating the new database.
@@ -104,12 +130,15 @@ public function create(Server $server, array $data)
             }
         }
 
-        // Max of 48 characters, including the s123_ that we append to the front.
-        $truncatedName = substr($data['database'], 0, 48 - strlen("s{$server->id}_"));
+        // Protect against developer mistakes...
+        if (empty($data['database']) || ! preg_match(self::MATCH_NAME_REGEX, $data['database'])) {
+            throw new InvalidArgumentException(
+                'The database name passed to DatabaseManagementService::handle MUST be prefixed with "s{server_id}_".'
+            );
+        }
 
         $data = array_merge($data, [
             'server_id' => $server->id,
-            'database' => $truncatedName,
             'username' => sprintf('u%d_%s', $server->id, str_random(10)),
             'password' => $this->encrypter->encrypt(
                 Utilities::randomStringWithSpecialCharacters(24)
@@ -120,7 +149,8 @@ public function create(Server $server, array $data)
 
         try {
             return $this->connection->transaction(function () use ($data, &$database) {
-                $database = $this->repository->createIfNotExists($data);
+                $database = $this->createModel($data);
+
                 $this->dynamic->set('dynamic', $data['database_host_id']);
 
                 $this->repository->createDatabase($database->database);
@@ -139,7 +169,7 @@ public function create(Server $server, array $data)
                     $this->repository->dropUser($database->username, $database->remote);
                     $this->repository->flush();
                 }
-            } catch (Exception $exception) {
+            } catch (Exception $deletionException) {
                 // Do nothing here. We've already encountered an issue before this point so no
                 // reason to prioritize this error over the initial one.
             }
@@ -166,4 +196,33 @@ public function delete(Database $database)
 
         return $database->delete();
     }
+
+    /**
+     * Create the database if there is not an identical match in the DB. While you can technically
+     * have the same name across multiple hosts, for the sake of keeping this logic easy to understand
+     * and avoiding user confusion we will ignore the specific host and just look across all hosts.
+     *
+     * @param array $data
+     * @return \Pterodactyl\Models\Database
+     *
+     * @throws \Pterodactyl\Exceptions\Repository\DuplicateDatabaseNameException
+     * @throws \Throwable
+     */
+    protected function createModel(array $data): Database
+    {
+        $exists = Database::query()->where('server_id', $data['server_id'])
+            ->where('database', $data['database'])
+            ->exists();
+
+        if ($exists) {
+            throw new DuplicateDatabaseNameException(
+                'A database with that name already exists for this server.'
+            );
+        }
+
+        $database = (new Database)->forceFill($data);
+        $database->saveOrFail();
+
+        return $database;
+    }
 }
diff --git a/app/Services/Databases/DeployServerDatabaseService.php b/app/Services/Databases/DeployServerDatabaseService.php
@@ -2,44 +2,27 @@
 
 namespace Pterodactyl\Services\Databases;
 
+use Webmozart\Assert\Assert;
 use Pterodactyl\Models\Server;
 use Pterodactyl\Models\Database;
-use Pterodactyl\Contracts\Repository\DatabaseRepositoryInterface;
-use Pterodactyl\Contracts\Repository\DatabaseHostRepositoryInterface;
+use Pterodactyl\Models\DatabaseHost;
 use Pterodactyl\Exceptions\Service\Database\NoSuitableDatabaseHostException;
 
 class DeployServerDatabaseService
 {
-    /**
-     * @var \Pterodactyl\Contracts\Repository\DatabaseHostRepositoryInterface
-     */
-    private $databaseHostRepository;
-
     /**
      * @var \Pterodactyl\Services\Databases\DatabaseManagementService
      */
     private $managementService;
 
-    /**
-     * @var \Pterodactyl\Contracts\Repository\DatabaseRepositoryInterface
-     */
-    private $repository;
-
     /**
      * ServerDatabaseCreationService constructor.
      *
-     * @param \Pterodactyl\Contracts\Repository\DatabaseRepositoryInterface $repository
-     * @param \Pterodactyl\Contracts\Repository\DatabaseHostRepositoryInterface $databaseHostRepository
      * @param \Pterodactyl\Services\Databases\DatabaseManagementService $managementService
      */
-    public function __construct(
-        DatabaseRepositoryInterface $repository,
-        DatabaseHostRepositoryInterface $databaseHostRepository,
-        DatabaseManagementService $managementService
-    ) {
-        $this->databaseHostRepository = $databaseHostRepository;
+    public function __construct(DatabaseManagementService $managementService)
+    {
         $this->managementService = $managementService;
-        $this->repository = $repository;
     }
 
     /**
@@ -53,28 +36,26 @@ public function __construct(
      */
     public function handle(Server $server, array $data): Database
     {
-        $allowRandom = config('pterodactyl.client_features.databases.allow_random');
-        $hosts = $this->databaseHostRepository->setColumns(['id'])->findWhere([
-            ['node_id', '=', $server->node_id],
-        ]);
+        Assert::notEmpty($data['database'] ?? null);
+        Assert::notEmpty($data['remote'] ?? null);
 
-        if ($hosts->isEmpty() && ! $allowRandom) {
+        $hosts = DatabaseHost::query()->get()->toBase();
+        if ($hosts->isEmpty()) {
             throw new NoSuitableDatabaseHostException;
-        }
+        } else {
+            $nodeHosts = $hosts->where('node_id', $server->node_id)->toBase();
 
-        if ($hosts->isEmpty()) {
-            $hosts = $this->databaseHostRepository->setColumns(['id'])->all();
-            if ($hosts->isEmpty()) {
+            if ($nodeHosts->isEmpty() && ! config('pterodactyl.client_features.databases.allow_random')) {
                 throw new NoSuitableDatabaseHostException;
             }
         }
 
-        $host = $hosts->random();
-
         return $this->managementService->create($server, [
-            'database_host_id' => $host->id,
-            'database' => array_get($data, 'database'),
-            'remote' => array_get($data, 'remote'),
+            'database_host_id' => $nodeHosts->isEmpty()
+                ? $hosts->random()->id
+                : $nodeHosts->random()->id,
+            'database' => DatabaseManagementService::generateUniqueDatabaseName($data['database'], $server->id),
+            'remote' => $data['remote'],
         ]);
     }
 }
diff --git a/tests/Integration/Services/Databases/DatabaseManagementServiceTest.php b/tests/Integration/Services/Databases/DatabaseManagementServiceTest.php
diff --git a/tests/Integration/Services/Databases/DeployServerDatabaseServiceTest.php b/tests/Integration/Services/Databases/DeployServerDatabaseServiceTest.php

Original file line number	Diff line number	Diff line change
`@@ -362,7 +362,7 @@ public function saveStartup(Request $request, Server $server)`
`362`	`362`	`public function newDatabase(StoreServerDatabaseRequest $request, Server $server)`
`363`	`363`	`{`
`364`	`364`	`$this->databaseManagementService->create($server, [`
`365`		`- 'database' => $request->input('database'),`
	`365`	`+ 'database' => DatabaseManagementService::generateUniqueDatabaseName($request->input('database'), $server->id),`
`366`	`366`	`'remote' => $request->input('remote'),`
`367`	`367`	`'database_host_id' => $request->input('database_host_id'),`
`368`	`368`	`'max_connections' => $request->input('max_connections'),`