Use filestreams (#1351)

* Started to refactor GdImageHandler

* Added exceptions

* Refactored interface

* Refactored GD handler

* Streams for all real image handlers

* Adopted image handlers to file classes.

* Fixed generic/automatic image handler.

* A lot of work

* Remove `checksum` from `Extractor`

* Cleaned up MOVFormat

* Refactored handling of Google Motion Pictures and finalized AddStandaloneStrategy.

* Made collection of stream statistics optional on save.

* Removed ImageHandler as a service.

* Fixed size variant factory and interface

* Use exception-free report in ImageHandler

* Repaired exception handling in AddStandaloneStrategy

* Fixed exception handling in RotateStrategy

* Fixed exceptions and checksum in AddPhotoPartnerStrategy

* Somehow unrelated refactoring of SizeVariantNamingStrategy

* Follow up of unrelated fix of naming strategy.

* Fixed AddVideoPartnerStrategy

* New naming strategies for size variants

* Switched to SizeVariantSharedPrefixRandomNamingStrategy

* fix-to-squash: Fixes in AddStandaloneStrategy

* Fixed unrelated error in test which suddenly fails

* fix-to-squash: Repaired type errors und undefined variables in ImgagickHandler

* Refactored tests

* More tests

* More tests

* Improved tests

* Remove dead code

* More tests

* Removed dead code

* Update dependencies due to bug in Laravel

* Fixed a lot of tests

* More tests

* Fixed test

* Uses helper method `public_path`

* Removed storage facade from test.

* Cleaned up hard-coded paths

* Make tests more pretty

* Added test for rotated photo and broken motion photo

* Added "Location" EXIF tag to sample

* Refactored Photo Rotation Tests

* Refactored upload-method

* More tests for photo rotation.

* Added Archive Tests

* More tests for archive

* Fixed SonarCloud issues

* Fixed typo

* Added test for import via download.

* Bugfix for falsely skipped image optimization.

* Added @ildyria's suggestion for a naming strategy

* Moved repeated test code into traits

* Refactored test for adding photos in preperation for testing GD handler.

* Fixed GD handler

* Added test for GD handler

* Fix in GD image handler

* Added different file types for test

* Added rotation tests for GD handler

* Added samples for all kind of auto-orientation.

* Removed dead code

* Removed absolute path from FlysystemFile.

* Bug fix for naming strategies wrt. raw variants, part I.

* Added negative test for unsupported raw.

* Bug fix for naming strategies wrt. raw variants, part II.

* Added positive tests for raw upload.

* Quick fix to track down error which only occurs with Github workflows

* Exchanged PDF with XCF as raw test case, because Githib workflows forbid PDF due to security concerns.

* Revert "Quick fix to track down error which only occurs with Github workflows"

This reverts commit 00e2c09.

* Exchanged XCF with TIFF as raw test case, because Githib craches for XCF.

* Test for import of supported/unsupported raw file via URL

* Make SonarCloud happy about string constants.

* Removed dead code

* Split command tests and make command tests runnable (renamed to ...Test).

* Fixed trivial bugs in commands

* Make Sonar Cloud happy.

* Make Sonar Cloud even more hapyy.

* First positive test for command `lychee:generate_thumbs`.

* Added test to re-create video thumbnail.

* Fixed error when re-creating a subset of missing size variants

* Test set creation time from file creation time.

* More tests (for ghostbuster)

* More tests (for ghostbuster), this time for real.

* Nuked unused size variants

* Fixed `makefile` as pointed out by @qwerty287

* Added stub for GD to make PHPStan happy.

* Apply suggestions from code review

Co-authored-by: Kamil Iskra <[email protected]>

* Re-phrased comment as suggested by code review.

* Remove stupid addition (sorry)

* Update app/Assets/SizeVariantGroupedWithRandomSuffixNamingStrategy.php

Co-authored-by: Kamil Iskra <[email protected]>

* Update app/Assets/SizeVariantGroupedWithRandomSuffixNamingStrategy.php

Co-authored-by: Kamil Iskra <[email protected]>

* Update app/Actions/Photo/Strategies/AddStandaloneStrategy.php

Co-authored-by: Matthias Nagel <[email protected]>

* Update app/Actions/Photo/Strategies/RotateStrategy.php

Co-authored-by: Kamil Iskra <[email protected]>

* Update app/Assets/SizeVariantGroupedWithRandomSuffixNamingStrategy.php

Co-authored-by: Kamil Iskra <[email protected]>

* Update app/Image/GdHandler.php

Co-authored-by: Kamil Iskra <[email protected]>

* Apply suggestions from code review

Co-authored-by: Kamil Iskra <[email protected]>

* Fix comment.

* Apply suggestions from code review

Co-authored-by: Kamil Iskra <[email protected]>

* Simplified code for compression quality

* Fixed condition for creation of size variants

* Updated comment on Quicktime container

* Merge saveFrame and extractFrame into one and removed optimization.

* Apply suggestions from code review

Co-authored-by: Kamil Iskra <[email protected]>

* Fixed subleties

* Fixed file permissions on uploading

* Fix efficiency regression for GD handler

* Fixed base test class to avoid crash with chmod

* Test for all size variants in simple upload

* Avoid failing tests due to incorrect file owner

* Efficiency boost for GD handler

* Added "dry-run" to console command "fix-permissions"

* better messages

* fix phpstan with useless default

* Update app/Console/Commands/FixPermissions.php

Co-authored-by: Kamil Iskra <[email protected]>

* Update app/Actions/Diagnostics/Checks/BasicPermissionCheck.php

Co-authored-by: Kamil Iskra <[email protected]>

* Update app/Image/GdHandler.php

Co-authored-by: Kamil Iskra <[email protected]>

* fix commands (because yes, I tested them, they didn't work)

* nice message if nothing to fix is required

* fix phpstan

* fix tests

* Moved command  from composer to migration

* Added disclaimer to end of command `fix-permissions`

* Make file permissions more configurable

* Added config option to `.env` file

* Update app/Actions/Diagnostics/Checks/BasicPermissionCheck.php

Co-authored-by: Kamil Iskra <[email protected]>

* Update .env.example

Co-authored-by: Kamil Iskra <[email protected]>

* Update app/Console/Commands/FixPermissions.php

Co-authored-by: Kamil Iskra <[email protected]>

* Made permission check more robust (no silent failure); no hard-coded disk name

* Repaired broken umask

* Fixed `neither ... nor` vs. `not ... or`

* Fixed "neither ... nor" vs. "not ... or"

* Added comment about bug for world-writeable directories

* Fixed unused parameter.

* bump to version 4.5.2

Co-authored-by: Kamil Iskra <[email protected]>
Co-authored-by: ildyria <[email protected]>
Co-authored-by: Benoît Viguier <[email protected]>

.env.example

@@ -35,6 +35,30 @@ DB_LOG_SQL=false
 # Don't use a timezone offset (like +01:00) or a timezone abbreviation (like CEST)
 # TIMEZONE=Europe/Paris
 
+# Visibility of directories and (media) files in LYCHEE_UPLOADS
+# Possible values are:
+#
+#  - private: world group has neither read nor write access
+#  - public: world group has read access but no write access (the default)
+#  - world: world group has read and write access
+#
+# The default should suffice for most installations.
+# For improved security, change this setting to "private".
+# Some rare setups may require directories and files to be world writeable.
+# In this case, use "world" here.
+# USE WITH PRECAUTIONS: world writeable files and folders may be a SECURITY RISK.
+# Note at the time of writing, the Flysystem package v1 has a bug which
+# prevents the "world" preset from working.
+# This is a temporary problem and will be solved after
+# https://github.com/thephpleague/flysystem/pull/1523 will have been merged
+# upstream or after Lychee will have migrated to Laravel 9.
+# Until then, if you need to use world-writable directories, please edit
+# `config/filesystems.php` and re-define the values for
+# `disks.images.permissions.(file|dir).public` such that they equal
+# `disks.images.permissions.(file|dir).world` and use the preset `public`
+# instead.
+# LYCHEE_IMAGE_VISIBILITY=public
+
 # folders in which the files will be stored
 # LYCHEE_DIST="/var/www/html/Lychee-Laravel/public/dist/"
 # LYCHEE_UPLOADS="/var/www/html/Lychee-Laravel/public/uploads/"

.gitignore

@@ -7,21 +7,9 @@ public/Lychee-front/node_modules/
 public/Lychee-front/bower_components/
 public/Lychee-front/package-lock.json
 
-public/uploads/big/*
-public/uploads/import/*
-public/uploads/medium/*
-public/uploads/raw/*
-public/uploads/small/*
-public/uploads/thumb/*
-public/uploads/tracks/*
-
-!public/uploads/big/index.html
-!public/uploads/import/index.html
-!public/uploads/medium/index.html
-!public/uploads/raw/index.html
-!public/uploads/small/index.html
-!public/uploads/thumb/index.html
-!public/uploads/tracks/index.html
+public/dist/user.css
+
+public/uploads/**
 
 /storage/*.key
 /storage/clockwork/
@@ -36,7 +24,6 @@ yarn-error.log
 .env
 .env.*
 
-public/dist/user.css
 aliases
 
 Lychee/*

app/Actions/Diagnostics/Checks/BasicPermissionCheck.php

@@ -3,27 +3,105 @@
 namespace App\Actions\Diagnostics\Checks;
 
 use App\Contracts\DiagnosticCheckInterface;
+use App\Contracts\SizeVariantNamingStrategy;
+use App\Exceptions\Handler;
+use App\Exceptions\Internal\InvalidConfigOption;
 use App\Facades\Helpers;
+use App\Models\SymLink;
+use Illuminate\Contracts\Container\BindingResolutionException;
+use Illuminate\Contracts\Filesystem\Filesystem;
 use Illuminate\Support\Facades\Storage;
+use League\Flysystem\Adapter\Local as LocalFlysystem;
+use Psr\Container\ContainerExceptionInterface;
+use Psr\Container\NotFoundExceptionInterface;
+use function Safe\sprintf;
 
 class BasicPermissionCheck implements DiagnosticCheckInterface
 {
+	public const MAX_ISSUE_REPORTS_PER_TYPE = 5;
+
+	/**
+	 * @var int[] IDs of all (POSIX) groups to which the process belongs
+	 */
+	protected array $groupIDs;
+
+	/**
+	 * @var string Comma-separated list of names of (POSIX) groups to which the process belongs
+	 */
+	protected string $groupNames;
+
+	protected int $numOwnerIssues;
+
+	protected int $numPermissionIssues;
+
+	protected int $numAccessIssues;
+
+	/**
+	 * @param string[] $errors
+	 *
+	 * @return void
+	 */
 	public function check(array &$errors): void
 	{
 		$this->folders($errors);
 		$this->userCSS($errors);
 	}
 
+	/**
+	 * @param string[] $errors
+	 *
+	 * @return void
+	 */
 	public function folders(array &$errors): void
 	{
-		$paths = ['big', 'medium', 'small', 'thumb', 'import', ''];
+		if (!extension_loaded('posix')) {
+			return;
+		}
 
-		foreach ($paths as $path) {
-			$p = Storage::path($path);
-			if (!Helpers::hasPermissions($p)) {
-				$errors[] = "Error: '" . $p . "' is missing or has insufficient read/write privileges";
+		clearstatcache(true);
+		$this->numOwnerIssues = 0;
+		$this->numPermissionIssues = 0;
+		$this->numAccessIssues = 0;
+		$groupIDsOrFalse = posix_getgroups();
+		if ($groupIDsOrFalse === false) {
+			$errors[] = 'Error: Could not determine groups of process';
+
+			return;
+		}
+		$this->groupIDs = $groupIDsOrFalse;
+		$this->groupIDs[] = posix_getegid();
+		$this->groupIDs[] = posix_getgid();
+		$this->groupIDs = array_unique($this->groupIDs);
+		$this->groupNames = implode(', ', array_map(
+			function (int $gid): string {
+				$groupNameOrFalse = posix_getgrgid($gid);
+
+				return $groupNameOrFalse === false ? '<unknown>' : $groupNameOrFalse['name'];
+			},
+			$this->groupIDs
+		));
+
+		/** @var Filesystem[] $disks */
+		$disks = [
+			SizeVariantNamingStrategy::getImageDisk(),
+			Storage::disk(SymLink::DISK_NAME),
+		];
+
+		foreach ($disks as $disk) {
+			if ($disk->getDriver()->getAdapter() instanceof LocalFlysystem) {
+				$this->checkDirectoryPermissionsRecursively($disk->path(''), $errors);
 			}
 		}
+
+		if ($this->numOwnerIssues > self::MAX_ISSUE_REPORTS_PER_TYPE) {
+			$errors[] = sprintf('Warning: %d more directories with wrong owner', $this->numOwnerIssues - self::MAX_ISSUE_REPORTS_PER_TYPE);
+		}
+		if ($this->numPermissionIssues > self::MAX_ISSUE_REPORTS_PER_TYPE) {
+			$errors[] = sprintf('Warning: %d more directories with wrong permissions', $this->numPermissionIssues - self::MAX_ISSUE_REPORTS_PER_TYPE);
+		}
+		if ($this->numAccessIssues > self::MAX_ISSUE_REPORTS_PER_TYPE) {
+			$errors[] = sprintf('Warning: %d more inaccessible directories', $this->numAccessIssues - self::MAX_ISSUE_REPORTS_PER_TYPE);
+		}
 	}
 
 	public function userCSS(array &$errors): void
@@ -37,4 +115,125 @@ public function userCSS(array &$errors): void
 			}
 		}
 	}
+
+	/**
+	 * Check permissions of (local) image directories.
+	 *
+	 * For efficiency reasons only the directory permissions are checked,
+	 * not the permissions of every single file.
+	 *
+	 * @param string   $path   the path of the directory or file to check
+	 * @param string[] $errors the list of errors to append to
+	 * @noinspection PhpComposerExtensionStubsInspection
+	 */
+	private function checkDirectoryPermissionsRecursively(string $path, array &$errors): void
+	{
+		try {
+			if (!is_dir($path)) {
+				return;
+			}
+
+			$actualPerm = fileperms($path);
+			if ($actualPerm === false) {
+				$errors[] = sprintf('Warning: Unable to determine permissions for %s' . PHP_EOL, $path);
+
+				return;
+			}
+
+			// `fileperms` also returns the higher bits of the inode mode.
+			// Hence, we must AND it with 07777 to only get what we are
+			// interested in
+			$actualPerm &= 07777;
+			$owningGroupIdOrFalse = filegroup($path);
+			$owningGroupNameOrFalse = $owningGroupIdOrFalse === false ? false : posix_getgrgid($owningGroupIdOrFalse);
+			$owningGroupName = $owningGroupNameOrFalse === false ? '<unknown>' : $owningGroupNameOrFalse['name'];
+			$expectedPerm = self::getConfiguredDirectoryPerm();
+
+			if (!in_array($owningGroupIdOrFalse, $this->groupIDs, true)) {
+				$this->numOwnerIssues++;
+				if ($this->numOwnerIssues <= self::MAX_ISSUE_REPORTS_PER_TYPE) {
+					$errors[] = sprintf('Warning: %s is owned by group %s, but should be owned by one out of %s', $path, $owningGroupName, $this->groupNames);
+				}
+			}
+
+			if ($expectedPerm !== $actualPerm) {
+				$this->numPermissionIssues++;
+				if ($this->numPermissionIssues <= self::MAX_ISSUE_REPORTS_PER_TYPE) {
+					$errors[] = sprintf(
+						'Warning: %s has permissions %04o, but should have %04o',
+						$path,
+						$actualPerm,
+						$expectedPerm
+					);
+				}
+			}
+
+			if (!is_writable($path) || !is_readable($path)) {
+				$this->numAccessIssues++;
+				if ($this->numAccessIssues <= self::MAX_ISSUE_REPORTS_PER_TYPE) {
+					$problem = match (true) {
+						(!is_writable($path) && !is_readable($path)) => 'neither readable nor writable',
+						!is_writable($path) => 'not writable',
+						!is_readable($path) => 'not readable',
+						default => ''
+					};
+					$errors[] = sprintf('Error: %s is %s by %s', $path, $problem, $this->groupNames);
+				}
+			}
+
+			$dir = new \DirectoryIterator($path);
+			foreach ($dir as $dirEntry) {
+				if ($dirEntry->isDir() && !$dirEntry->isDot()) {
+					$this->checkDirectoryPermissionsRecursively($dirEntry->getPathname(), $errors);
+				}
+			}
+		} catch (\Exception $e) {
+			$errors[] = 'Error: ' . $e->getMessage();
+			Handler::reportSafely($e);
+		}
+	}
+
+	/**
+	 * @throws InvalidConfigOption
+	 */
+	public static function getConfiguredDirectoryPerm(): int
+	{
+		return self::getConfiguredPerm('dir');
+	}
+
+	/**
+	 * @throws InvalidConfigOption
+	 */
+	public static function getConfiguredFilePerm(): int
+	{
+		return self::getConfiguredPerm('file');
+	}
+
+	/**
+	 * @param string $type either 'dir' or 'file'
+	 *
+	 * @return int
+	 *
+	 * @phpstan-param 'dir'|'file' $type
+	 *
+	 * @throws InvalidConfigOption
+	 */
+	private static function getConfiguredPerm(string $type): int
+	{
+		try {
+			$visibility = (string) config(sprintf('filesystems.disks.%s.visibility', SizeVariantNamingStrategy::IMAGE_DISK_NAME));
+			if ($visibility === '') {
+				throw new InvalidConfigOption('File/directory visibility not configured');
+			}
+
+			$perm = (int) config(sprintf('filesystems.disks.%s.permissions.%s.%s', SizeVariantNamingStrategy::IMAGE_DISK_NAME, $type, $visibility));
+			if ($perm === 0) {
+				throw new InvalidConfigOption('Configured file/directory permission is invalid');
+			}
+
+			return $perm;
+		} catch (ContainerExceptionInterface|BindingResolutionException|NotFoundExceptionInterface $e) {
+			throw new InvalidConfigOption('Could not read configuration for file/directory permission', $e);
+		}
+	}
 }

app/Actions/Diagnostics/Errors.php

@@ -20,7 +20,7 @@ public function __construct(DiagnosticsChecksFactory $diagnosticsChecksFactory)
 	 */
 	public function get(): array
 	{
-		// Declare
+		/** @var string[] $errors */
 		$errors = [];
 
 		// @codeCoverageIgnoreStart

app/Actions/Import/FromUrl.php

@@ -2,11 +2,9 @@
 
 namespace App\Actions\Import;
 
-use App\Actions\Import\Extensions\Checks;
 use App\Actions\Photo\Create;
 use App\Actions\Photo\Strategies\ImportMode;
 use App\Exceptions\Handler;
-use App\Exceptions\InsufficientFilesystemPermissions;
 use App\Exceptions\MassImportException;
 use App\Image\DownloadedFile;
 use App\Image\MediaFile;
@@ -21,19 +19,6 @@
 
 class FromUrl
 {
-	use Checks;
-
-	/**
-	 * @throws InsufficientFilesystemPermissions
-	 */
-	public function __construct()
-	{
-		// TODO: Why do we explicitly perform this check here? We don't check the other import classes. We could just let the import fail.
-		// Moreover, we do not even use the `import` folder which is checked by this method.
-		// There is similar odd test in {@link \App\Actions\Photo\Create::add()} which uses another "check" trait.
-		$this->checkPermissions();
-	}
-
 	/**
 	 * Imports photos from a list of URLs.
 	 *