From d15165fec0eaef514b09ed5be57491b5302d8712 Mon Sep 17 00:00:00 2001 From: Frank de Jonge Date: Thu, 2 Nov 2023 20:11:14 +0100 Subject: [PATCH 1/3] Allow copy without retaining visibility for adapters and implementations that required fetching visibility for copy and move. --- src/AsyncAwsS3/AsyncAwsS3Adapter.php | 9 ++-- src/AwsS3V3/AwsS3V3Adapter.php | 9 ++-- src/Config.php | 10 ++++ src/Ftp/FtpAdapter.php | 16 ++++-- .../GoogleCloudStorageAdapter.php | 12 +++-- src/MountManager.php | 53 ++++++++++++------- src/PhpseclibV3/SftpAdapter.php | 9 +++- 7 files changed, 85 insertions(+), 33 deletions(-) diff --git a/src/AsyncAwsS3/AsyncAwsS3Adapter.php b/src/AsyncAwsS3/AsyncAwsS3Adapter.php index 85fe67c2c..12b200b70 100644 --- a/src/AsyncAwsS3/AsyncAwsS3Adapter.php +++ b/src/AsyncAwsS3/AsyncAwsS3Adapter.php @@ -315,14 +315,17 @@ public function move(string $source, string $destination, Config $config): void public function copy(string $source, string $destination, Config $config): void { try { - /** @var string $visibility */ - $visibility = $config->get(Config::OPTION_VISIBILITY) ?: $this->visibility($source)->visibility(); + $visibility = $config->get(Config::OPTION_VISIBILITY); + + if ($visibility === null && $config->get('retain_visibility', true)) { + $visibility = $this->visibility($source)->visibility(); + } } catch (Throwable $exception) { throw UnableToCopyFile::fromLocationTo($source, $destination, $exception); } $arguments = [ - 'ACL' => $this->visibility->visibilityToAcl($visibility), + 'ACL' => $this->visibility->visibilityToAcl($visibility ?: 'private'), 'Bucket' => $this->bucket, 'Key' => $this->prefixer->prefixPath($destination), 'CopySource' => $this->bucket . '/' . $this->prefixer->prefixPath($source), diff --git a/src/AwsS3V3/AwsS3V3Adapter.php b/src/AwsS3V3/AwsS3V3Adapter.php index 9b87d8c66..9e35a50e8 100644 --- a/src/AwsS3V3/AwsS3V3Adapter.php +++ b/src/AwsS3V3/AwsS3V3Adapter.php @@ -415,8 +415,11 @@ public function move(string $source, string $destination, Config $config): void public function copy(string $source, string $destination, Config $config): void { try { - /** @var string $visibility */ - $visibility = $config->get(Config::OPTION_VISIBILITY) ?: $this->visibility($source)->visibility(); + $visibility = $config->get(Config::OPTION_VISIBILITY); + + if ($visibility === null && $config->get('retain_visibility', true)) { + $visibility = $this->visibility($source)->visibility(); + } } catch (Throwable $exception) { throw UnableToCopyFile::fromLocationTo( $source, @@ -431,7 +434,7 @@ public function copy(string $source, string $destination, Config $config): void $this->prefixer->prefixPath($source), $this->bucket, $this->prefixer->prefixPath($destination), - $this->visibility->visibilityToAcl($visibility), + $this->visibility->visibilityToAcl($visibility ?: 'private'), $this->createOptionsFromConfig($config)['params'] ); } catch (Throwable $exception) { diff --git a/src/Config.php b/src/Config.php index a416c67e0..691f9b63d 100644 --- a/src/Config.php +++ b/src/Config.php @@ -36,4 +36,14 @@ public function withDefaults(array $defaults): Config { return new Config($this->options + $defaults); } + + public function toArray(): array + { + return $this->options; + } + + public function withSetting(string $property, mixed $setting): Config + { + return $this->extend([$property => $setting]); + } } diff --git a/src/Ftp/FtpAdapter.php b/src/Ftp/FtpAdapter.php index 0127df357..d44e4b2ce 100644 --- a/src/Ftp/FtpAdapter.php +++ b/src/Ftp/FtpAdapter.php @@ -547,7 +547,7 @@ public function move(string $source, string $destination, Config $config): void $connection = $this->connection(); if ( ! @ftp_rename($connection, $sourceLocation, $destinationLocation)) { - throw UnableToMoveFile::fromLocationTo($source, $destination); + throw UnableToMoveFile::because(error_get_last()['message'] ?? 'reason unknown', $source, $destination); } } @@ -555,8 +555,13 @@ public function copy(string $source, string $destination, Config $config): void { try { $readStream = $this->readStream($source); - $visibility = $this->visibility($source)->visibility(); - $this->writeStream($destination, $readStream, new Config(compact('visibility'))); + $visibility = $config->get(Config::OPTION_VISIBILITY); + + if ($visibility === null && $config->get('retain_visibility', true)) { + $config = $config->withSetting(Config::OPTION_VISIBILITY, $this->visibility($source)->visibility()); + } + + $this->writeStream($destination, $readStream, $config); } catch (Throwable $exception) { if (isset($readStream) && is_resource($readStream)) { @fclose($readStream); @@ -604,7 +609,10 @@ private function ensureDirectoryExists(string $dirname, ?string $visibility): vo } if ($mode !== false && @ftp_chmod($connection, $mode, $location) === false) { - throw UnableToCreateDirectory::atLocation($dirPath, 'unable to chmod the directory'); + throw UnableToCreateDirectory::atLocation( + $dirPath, + 'unable to chmod the directory: ' . error_get_last()['message'] ?? 'reason unknown' + ); } } } diff --git a/src/GoogleCloudStorage/GoogleCloudStorageAdapter.php b/src/GoogleCloudStorage/GoogleCloudStorageAdapter.php index 74613a2b5..f152fbc92 100644 --- a/src/GoogleCloudStorage/GoogleCloudStorageAdapter.php +++ b/src/GoogleCloudStorage/GoogleCloudStorageAdapter.php @@ -348,11 +348,17 @@ public function move(string $source, string $destination, Config $config): void public function copy(string $source, string $destination, Config $config): void { try { - /** @var string $visibility */ - $visibility = $this->visibility($source)->visibility(); + $visibility = $config->get(Config::OPTION_VISIBILITY); + + if ($visibility === null && $config->get('retain_visibility', true)) { + $visibility = $this->visibility($source)->visibility(); + } + $prefixedSource = $this->prefixer->prefixPath($source); $options = ['name' => $this->prefixer->prefixPath($destination)]; - $predefinedAcl = $this->visibilityHandler->visibilityToPredefinedAcl($visibility); + $predefinedAcl = $this->visibilityHandler->visibilityToPredefinedAcl( + $visibility ?: PortableVisibilityHandler::NO_PREDEFINED_VISIBILITY + ); if ($predefinedAcl !== PortableVisibilityHandler::NO_PREDEFINED_VISIBILITY) { $options['predefinedAcl'] = $predefinedAcl; diff --git a/src/MountManager.php b/src/MountManager.php index 4f4ad5516..3dcfd234c 100644 --- a/src/MountManager.php +++ b/src/MountManager.php @@ -17,14 +17,20 @@ class MountManager implements FilesystemOperator */ private $filesystems = []; + /** + * @var Config + */ + private $config; + /** * MountManager constructor. * * @param array $filesystems */ - public function __construct(array $filesystems = []) + public function __construct(array $filesystems = [], array $config = []) { $this->mountFilesystems($filesystems); + $this->config = new Config($config); } public function fileExists(string $location): bool @@ -156,7 +162,7 @@ public function write(string $location, string $contents, array $config = []): v [$filesystem, $path] = $this->determineFilesystemAndPath($location); try { - $filesystem->write($path, $contents, $config); + $filesystem->write($path, $contents, $this->config->extend($config)->toArray()); } catch (UnableToWriteFile $exception) { throw UnableToWriteFile::atLocation($location, $exception->reason(), $exception); } @@ -166,7 +172,7 @@ public function writeStream(string $location, $contents, array $config = []): vo { /** @var FilesystemOperator $filesystem */ [$filesystem, $path] = $this->determineFilesystemAndPath($location); - $filesystem->writeStream($path, $contents, $config); + $filesystem->writeStream($path, $contents, $this->config->extend($config)->toArray()); } public function setVisibility(string $path, string $visibility): void @@ -206,7 +212,7 @@ public function createDirectory(string $location, array $config = []): void [$filesystem, $path] = $this->determineFilesystemAndPath($location); try { - $filesystem->createDirectory($path, $config); + $filesystem->createDirectory($path, $this->config->extend($config)->toArray()); } catch (UnableToCreateDirectory $exception) { throw UnableToCreateDirectory::dueToFailure($location, $exception); } @@ -224,7 +230,8 @@ public function move(string $source, string $destination, array $config = []): v $sourcePath, $destinationPath, $source, - $destination + $destination, + $config, ) : $this->moveAcrossFilesystems($source, $destination, $config); } @@ -240,15 +247,16 @@ public function copy(string $source, string $destination, array $config = []): v $sourcePath, $destinationPath, $source, - $destination + $destination, + $config, ) : $this->copyAcrossFilesystem( - $config['visibility'] ?? null, $sourceFilesystem, $sourcePath, $destinationFilesystem, $destinationPath, $source, - $destination + $destination, + $config, ); } @@ -273,7 +281,7 @@ public function temporaryUrl(string $path, DateTimeInterface $expiresAt, array $ throw new UnableToGenerateTemporaryUrl(sprintf('%s does not support generating public urls.', $filesystem::class), $path); } - return $filesystem->temporaryUrl($path, $expiresAt, $config); + return $filesystem->temporaryUrl($path, $expiresAt, $this->config->extend($config)->toArray()); } public function checksum(string $path, array $config = []): string @@ -285,7 +293,7 @@ public function checksum(string $path, array $config = []): string throw new UnableToProvideChecksum(sprintf('%s does not support providing checksums.', $filesystem::class), $path); } - return $filesystem->checksum($path, $config); + return $filesystem->checksum($path, $this->config->extend($config)->toArray()); } private function mountFilesystems(array $filesystems): void @@ -345,28 +353,36 @@ private function copyInSameFilesystem( string $sourcePath, string $destinationPath, string $source, - string $destination + string $destination, + array $config, ): void { try { - $sourceFilesystem->copy($sourcePath, $destinationPath); + $sourceFilesystem->copy($sourcePath, $destinationPath, $this->config->extend($config)->toArray()); } catch (UnableToCopyFile $exception) { throw UnableToCopyFile::fromLocationTo($source, $destination, $exception); } } private function copyAcrossFilesystem( - ?string $visibility, FilesystemOperator $sourceFilesystem, string $sourcePath, FilesystemOperator $destinationFilesystem, string $destinationPath, string $source, - string $destination + string $destination, + array $config, ): void { + $config = $this->config->extend($config); + $retainVisibility = (bool) $config->get('retain_visibility', true); + $visibility = $config->get('visibility'); + try { - $visibility = $visibility ?? $sourceFilesystem->visibility($sourcePath); + if ($visibility == null && $retainVisibility) { + $visibility = $sourceFilesystem->visibility($sourcePath); + } + $stream = $sourceFilesystem->readStream($sourcePath); - $destinationFilesystem->writeStream($destinationPath, $stream, compact('visibility')); + $destinationFilesystem->writeStream($destinationPath, $stream, $visibility ? compact('visibility') : []); } catch (UnableToRetrieveMetadata | UnableToReadFile | UnableToWriteFile $exception) { throw UnableToCopyFile::fromLocationTo($source, $destination, $exception); } @@ -377,10 +393,11 @@ private function moveInTheSameFilesystem( string $sourcePath, string $destinationPath, string $source, - string $destination + string $destination, + array $config, ): void { try { - $sourceFilesystem->move($sourcePath, $destinationPath); + $sourceFilesystem->move($sourcePath, $destinationPath, $this->config->extend($config)->toArray()); } catch (UnableToMoveFile $exception) { throw UnableToMoveFile::fromLocationTo($source, $destination, $exception); } diff --git a/src/PhpseclibV3/SftpAdapter.php b/src/PhpseclibV3/SftpAdapter.php index e10087fcc..301a1bfb8 100644 --- a/src/PhpseclibV3/SftpAdapter.php +++ b/src/PhpseclibV3/SftpAdapter.php @@ -327,8 +327,13 @@ public function copy(string $source, string $destination, Config $config): void { try { $readStream = $this->readStream($source); - $visibility = $this->visibility($source)->visibility(); - $this->writeStream($destination, $readStream, new Config(compact('visibility'))); + $visibility = $config->get(Config::OPTION_VISIBILITY); + + if ($visibility === null && $config->get('retain_visibility', true)) { + $config = $config->withSetting(Config::OPTION_VISIBILITY, $this->visibility($source)->visibility()); + } + + $this->writeStream($destination, $readStream, $config); } catch (Throwable $exception) { if (isset($readStream) && is_resource($readStream)) { @fclose($readStream); From b0159954accaeb1b30ca4521e893c795ffc039c3 Mon Sep 17 00:00:00 2001 From: Frank de Jonge Date: Thu, 2 Nov 2023 20:16:33 +0100 Subject: [PATCH 2/3] Tested MountManager for not retaining visibility --- src/MountManagerTest.php | 44 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/src/MountManagerTest.php b/src/MountManagerTest.php index cde9ebd0c..108d6da94 100644 --- a/src/MountManagerTest.php +++ b/src/MountManagerTest.php @@ -54,6 +54,50 @@ protected function setUp(): void ]); } + /** + * @test + */ + public function copying_without_retaining_visibility(): void + { + // arrange + $firstFilesystemAdapter = new InMemoryFilesystemAdapter(); + $secondFilesystemAdapter = new InMemoryFilesystemAdapter(); + $mountManager = new MountManager([ + 'first' => new Filesystem($firstFilesystemAdapter, ['visibility' => 'public']), + 'second' => new Filesystem($secondFilesystemAdapter, ['visibility' => 'private']), + ], ['retain_visibility' => false]); + + // act + $mountManager->write('first://file.txt', 'contents'); + $mountManager->copy('first://file.txt', 'second://file.txt'); + + // assert + $visibility = $mountManager->visibility('second://file.txt'); + self::assertEquals('private', $visibility); + } + + /** + * @test + */ + public function copying_while_retaining_visibility(): void + { + // arrange + $firstFilesystemAdapter = new InMemoryFilesystemAdapter(); + $secondFilesystemAdapter = new InMemoryFilesystemAdapter(); + $mountManager = new MountManager([ + 'first' => new Filesystem($firstFilesystemAdapter, ['visibility' => 'public']), + 'second' => new Filesystem($secondFilesystemAdapter, ['visibility' => 'private']), + ], ['retain_visibility' => true]); + + // act + $mountManager->write('first://file.txt', 'contents'); + $mountManager->copy('first://file.txt', 'second://file.txt'); + + // assert + $visibility = $mountManager->visibility('second://file.txt'); + self::assertEquals('public', $visibility); + } + /** * @test */ From 88beda96de957b9855123c23a8d13168a7758dce Mon Sep 17 00:00:00 2001 From: Frank de Jonge Date: Thu, 2 Nov 2023 20:17:51 +0100 Subject: [PATCH 3/3] Fix error message syntax. --- src/Ftp/FtpAdapter.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Ftp/FtpAdapter.php b/src/Ftp/FtpAdapter.php index d44e4b2ce..4b63d4eab 100644 --- a/src/Ftp/FtpAdapter.php +++ b/src/Ftp/FtpAdapter.php @@ -611,7 +611,7 @@ private function ensureDirectoryExists(string $dirname, ?string $visibility): vo if ($mode !== false && @ftp_chmod($connection, $mode, $location) === false) { throw UnableToCreateDirectory::atLocation( $dirPath, - 'unable to chmod the directory: ' . error_get_last()['message'] ?? 'reason unknown' + 'unable to chmod the directory: ' . (error_get_last()['message'] ?? 'reason unknown'), ); } }