fix Installation (LycheeOrg#1495)

* fix Installation
* require `database/database.sqlite` to be writable to avoid surprises when running the migration
* remove public/img requirement
* `database` also need to be writable and executable otherwise sqlite consider the database as readonly
* Removed need for file NO_SECURE_KEY
* Update app/Exceptions/Handler.php

Co-authored-by: Matthias Nagel <matthias.h.nagel@posteo.de>

app/Actions/Install/ApplyMigration.php

@@ -6,7 +6,8 @@
 use App\Exceptions\Internal\FrameworkException;
 use Illuminate\Contracts\Container\BindingResolutionException;
 use Illuminate\Support\Facades\Artisan;
-use function Safe\unlink;
+use Psr\Container\ContainerExceptionInterface;
+use Psr\Container\NotFoundExceptionInterface;
 
 class ApplyMigration
 {
@@ -73,26 +74,14 @@ public function keyGenerate(array &$output): void
 		try {
 			Artisan::call('key:generate', ['--force' => true]);
 			$this->str_to_array(Artisan::output(), $output);
-			if (!str_contains(end($output), 'Application key set successfully')) {
+			if (
+				!str_contains(end($output), 'Application key set successfully') ||
+				config('app.key') === null
+			) {
 				$output[] = 'We could not generate the encryption key.';
 				throw new InstallationFailedException('Could not generate encryption key');
 			}
-
-			// key is generated, we can safely remove that file (in theory)
-			$filename = base_path('.NO_SECURE_KEY');
-			if (file_exists($filename)) {
-				if (is_file($filename)) {
-					unlink($filename);
-				} else {
-					throw new InstallationFailedException('A filesystem object . ' . $filename . ' exists, but is not an ordinary file.');
-				}
-			}
-		} catch (\ErrorException $e) {
-			// possibly thrown by `unlink`
-			$output[] = $e->getMessage();
-			$output[] = 'Could not remove file `.NO_SECURE_KEY`.';
-			throw new InstallationFailedException('Could not remove file `.NO_SECURE_KEY`', $e);
-		} catch (BindingResolutionException $e) {
+		} catch (BindingResolutionException|NotFoundExceptionInterface|ContainerExceptionInterface $e) {
 			throw new FrameworkException('Laravel\'s container component', $e);
 		}
 	}

app/Actions/Install/DefaultConfig.php

@@ -54,14 +54,15 @@ class DefaultConfig
 			*/
 		'permissions' => [
 			'.' => 'file_exists|is_readable|is_writable|is_executable',
+			'database/' => 'file_exists|is_readable|is_writable|is_executable',
+			'database/database.sqlite' => 'file_exists|is_readable|is_writable',
 			'storage/framework/' => 'file_exists|is_readable|is_writable|is_executable',
 			'storage/framework/views/' => 'file_exists|is_readable|is_writable|is_executable',
 			'storage/framework/cache/' => 'file_exists|is_readable|is_writable|is_executable',
 			'storage/framework/sessions/' => 'file_exists|is_readable|is_writable|is_executable',
 			'storage/logs/' => 'file_exists|is_readable|is_writable|is_executable',
 			'bootstrap/cache/' => 'file_exists|is_readable|is_writable|is_executable',
 			'public/dist/' => 'file_exists|is_readable|is_writable|is_executable',
-			'public/img/' => 'file_exists|is_readable|is_writable|is_executable',
 			'public/sym/' => 'file_exists|is_readable|is_writable|is_executable',
 			'public/uploads/' => 'file_exists|is_readable|is_writable|is_executable',
 		],

app/Console/Commands/FixPermissions.php

@@ -4,18 +4,14 @@
 
 use App\Actions\Diagnostics\Checks\BasicPermissionCheck;
 use Illuminate\Console\Command;
+use Illuminate\Support\Facades\Storage;
 use function Safe\chmod;
 use function Safe\fileowner;
 use function Safe\sprintf;
 use Symfony\Component\Console\Exception\InvalidArgumentException;
 
 class FixPermissions extends Command
 {
-	public const DIRECTORIES = [
-		'public/uploads',
-		'public/sym',
-	];
-
 	/**
 	 * The name and signature of the console command.
 	 *
@@ -52,6 +48,11 @@ class FixPermissions extends Command
 	 */
 	public function handle(): int
 	{
+		$directories = [
+			Storage::disk('images')->path(''),
+			Storage::disk('symbolic')->path(''),
+		];
+
 		if (!extension_loaded('posix')) {
 			$this->error('Non-POSIX OS detected: Command unsupported');
 
@@ -63,7 +64,7 @@ public function handle(): int
 		clearstatcache(true);
 		$this->effUserId = posix_geteuid();
 
-		foreach (self::DIRECTORIES as $directory) {
+		foreach ($directories as $directory) {
 			$this->line(sprintf('Scanning: <info>%s</info>', $directory));
 			$this->fixPermissionsRecursively($directory);
 		}

app/Exceptions/Handler.php

@@ -10,13 +10,60 @@
 use App\Exceptions\Handlers\NoEncryptionKey;
 use App\Models\Logs;
 use Illuminate\Auth\AuthenticationException;
+use Illuminate\Contracts\Container\BindingResolutionException;
 use Illuminate\Contracts\Container\Container;
 use Illuminate\Foundation\Exceptions\Handler as ExceptionHandler;
+use Illuminate\Http\Request;
 use Illuminate\Session\TokenMismatchException;
 use Illuminate\Support\Arr;
+use Psr\Container\ContainerExceptionInterface;
+use Psr\Container\NotFoundExceptionInterface;
 use Symfony\Component\HttpFoundation\Response as SymfonyResponse;
+use Symfony\Component\HttpKernel\Exception\HttpException;
 use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface;
 
+/**
+ * Lychee's custom exception handler.
+ *
+ * While the overall architectural approach of the original exception handler
+ * of the framework is fine, the original exception handler is mostly broken
+ * when it comes to the details.
+ *
+ * The overall architectural approach is as follows:
+ *
+ *  1. Substitute or wrap certain exceptions by or into other exceptions
+ *     (i.e. via `mapException`)
+ *  2. Decide whether the client expects an HTML or JSON response
+ *  3. Convert (or "render") the exception into a response with said content
+ *     type
+ *
+ * However, there are two major issues with the original exception handler:
+ *
+ *  - Substitution of exception is not limited to `mapException` but happens
+ *    all the time which makes it hard to reliably predict what happens to
+ *    an exception when a method (other than `mapException`) is called.
+ *    One might end up with a different exception.
+ *    Moreover, not all of these substitution are sensible enough to add the
+ *    original exception as a predecessor to the new exception.
+ *  - A constant mix-up of the terms "HTTP" and "HTML".
+ *    The framework frequently uses the term "HTTP" as an antonym to "JSON"
+ *    when "HTML" would be rather appropriate.
+ *    For example like in `renderJsonResponse($e)` vs. `renderHttpResponse($e)`.
+ *    The latter is called, when an exception shall be converted into HTML.
+ *    But of course, a JSON response is also an HTTP response.
+ *    It seems as if the framework is not even aware of this confusion.
+ *
+ * 90% of this handler are bug fixes.
+ * This means, parent methods are not overwritten, because we need a special
+ * non-standard behaviour, but simply the _right_ behaviour.
+ * Unfortunately, this class cannot solve the unfortunate naming of some
+ * methods, but must stick to the names used by the parent class.
+ * Alternatively, this class could overwrite the entry method `render($e)`,
+ * re-implement everything which comes after that (even using better names)
+ * and let the rest of the parent class go down the drain.
+ * However, this bears the risk that some 3rd-party calls unfixed methods of
+ * the original exception handler.
+ */
 class Handler extends ExceptionHandler
 {
 	/**
@@ -153,12 +200,61 @@ protected function mapException(\Throwable $e): \Throwable
 	}
 
 	/**
-	 * Renders the given HttpException.
+	 * Prepare a response for the given exception.
+	 *
+	 * This method is called by the framework, _after_ the framework has
+	 * decided that the client expects a HTML response, but _before_ the
+	 * actual work horse {@link Handler::renderHttpException} is called.
+	 *
+	 * This method is 99% identical to the parent method except for a tiny
+	 * bug fix which adds the original exception to the encapsulating
+	 * `HttpException`.
+	 *
+	 * @param Request    $request
+	 * @param \Throwable $e
+	 *
+	 * @return SymfonyResponse
+	 *
+	 * @throws BindingResolutionException
+	 * @throws \InvalidArgumentException
+	 * @throws ContainerExceptionInterface
+	 * @throws NotFoundExceptionInterface
+	 */
+	protected function prepareResponse($request, \Throwable $e): SymfonyResponse
+	{
+		if (!$this->isHttpException($e) && config('app.debug') === true) {
+			return $this->toIlluminateResponse($this->convertExceptionToResponse($e), $e);
+		}
+
+		if (!$this->isHttpException($e)) {
+			$e = new HttpException(500, $e->getMessage(), $e);
+		}
+
+		// `renderHttpException` expects `$e` to be an instance of
+		// `HttpExceptionInterface`.
+		// This is ensured by `isHttpException` above, but PHPStan does not
+		// understand that.
+		// @phpstan-ignore-next-line
+		return $this->toIlluminateResponse($this->renderHttpException($e), $e);
+	}
+
+	/**
+	 * Renders the given HttpException into HTML.
 	 *
 	 * This method is called by the framework if
 	 *  1. `config('app.debug')` is not set, i.e. the application is not in debug mode
 	 *  2. the client expects an HTML response
 	 *
+	 * **Attention:**
+	 * This method is a misnomer caused by the framework.
+	 * The framework provides two methods `renderHttpException` and
+	 * `renderJsonException` with the former being called if the client
+	 * expects HTML.
+	 * Hence, the method should rather be named `renderHtmlException`.
+	 * That current name of the method, if meant as an antonym to
+	 * `renderJsonException` is obviously nonsense as JSON is also transported
+	 * over HTTP.
+	 *
 	 * @param HttpExceptionInterface $e
 	 *
 	 * @return SymfonyResponse

app/Exceptions/Handlers/NoEncryptionKey.php

@@ -5,7 +5,6 @@
 use App\Contracts\HttpExceptionHandler;
 use App\Redirections\ToInstall;
 use Illuminate\Encryption\MissingAppKeyException;
-use function Safe\touch;
 use Symfony\Component\HttpFoundation\Response as SymfonyResponse;
 use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface as HttpException;
 use Throwable;
@@ -37,7 +36,6 @@ public function check(HttpException $e): bool
 	public function renderHttpException(SymfonyResponse $defaultResponse, HttpException $e): SymfonyResponse
 	{
 		try {
-			touch(base_path('.NO_SECURE_KEY'));
 			$redirectResponse = ToInstall::go();
 			$contentType = $defaultResponse->headers->get('Content-Type');
 			if ($contentType !== null && $contentType !== '') {

app/Http/Middleware/Checks/IsInstalled.php

@@ -6,7 +6,11 @@
 use App\Contracts\MiddlewareCheck;
 use App\Exceptions\Internal\FrameworkException;
 use Illuminate\Contracts\Container\BindingResolutionException;
+use Illuminate\Database\QueryException;
 use Illuminate\Support\Facades\Schema;
+use Illuminate\Support\Str;
+use Psr\Container\ContainerExceptionInterface;
+use Psr\Container\NotFoundExceptionInterface;
 
 class IsInstalled implements MiddlewareCheck
 {
@@ -17,10 +21,25 @@ public function assert(): bool
 	{
 		try {
 			return
-				!file_exists(base_path('.NO_SECURE_KEY')) &&
+				config('app.key') !== null &&
 				Schema::hasTable('configs');
-		} catch (BindingResolutionException $e) {
+		} catch (QueryException $e) {
+			// Authentication to DB failled.
+			// This means that we cannot even check that `configs` is present,
+			// therefore we will just assume it is not.
+			//
+			// This can only happen if:
+			// - Connection with DB is broken (firewall?)
+			// - Connection with DB is not set (MySql without credentials)
+			//
+			// We only check Authentication to DB failled and just skip in
+			// the other cases to get a proper message error.
+			if (Str::contains($e->getMessage(), 'SQLSTATE[HY000] [1045]')) {
+				return false;
+			}
+			throw $e;
+		} catch (BindingResolutionException|NotFoundExceptionInterface|ContainerExceptionInterface $e) {
 			throw new FrameworkException('Laravel\'s container component', $e);
 		}
 	}
-}
+}

resources/views/install/permissions.blade.php

@@ -17,8 +17,21 @@
     @foreach ($permissions as $permission)
         <li class="list__item list__item--permissions">
         <span>{{ $permission['folder'] }}</span>
+        @if (count($permission['permission']) < 4)
+            @for ($i = 0;$i < 4 - count($permission['permission']); $i++)
+                <span class="perm float-right">
+                    &nbsp;
+                </span>
+            @endfor
+        @endif
         @foreach ($permission['permission'] as $perm)
-            @include('install.permission-line')
+            <span class="perm float-right">
+            @if($perm[1] & 1)
+                <i class="fa fa-fw fa-exclamation-circle error"></i>
+            @else
+                <i class="fa fa-fw fa-check-circle-o success"></i>
+            @endif
+            {{ $perm[0] }}</span>
         @endforeach
     @endforeach
     </ul>

tests/Feature/InstallTest.php

@@ -32,13 +32,16 @@ public function testInstall(): void
 		/** @var User $admin */
 		$admin = User::query()->find(0);
 
-		touch(base_path('.NO_SECURE_KEY'));
+		$prevAppKey = config('app.key');
+		config(['app.key' => null]);
 		$response = $this->get('install/');
 		$response->assertOk();
-		unlink(base_path('.NO_SECURE_KEY'));
+		config(['app.key' => $prevAppKey]);
 
 		// TODO: Why does a `git pull` delete `installed.log`? This test needs to be discussed with @ildyria
-		unlink(base_path('installed.log'));
+		if (file_exists(base_path('installed.log'))) {
+			unlink(base_path('installed.log'));
+		}
 		/**
 		 * No installed.log: we should not be redirected to install (case where we have not done the last migration).
 		 */