From cb0794f4677e8079b273f9b59df8c75b0815dd81 Mon Sep 17 00:00:00 2001 From: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> Date: Wed, 14 Oct 2026 16:04:30 +0200 Subject: [PATCH 1/2] fix: allow renames of partial files The new behaviour is needed when the partial files are not uploaded directly at the target directory, which is a configurable option (part_file_in_storage=false). Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> --- .../Storage/Wrapper/DirPermissionsMask.php | 11 +- .../Files/Storage/Wrapper/PermissionsMask.php | 14 +- lib/private/Files/Utils/PartialFileHelper.php | 19 +++ .../Wrapper/DirPermissionsMaskTest.php | 142 ++++++++++++++++++ .../Storage/Wrapper/PermissionsMaskTest.php | 55 +++++++ 5 files changed, 225 insertions(+), 16 deletions(-) create mode 100644 lib/private/Files/Utils/PartialFileHelper.php create mode 100644 tests/lib/Files/Storage/Wrapper/DirPermissionsMaskTest.php diff --git a/lib/private/Files/Storage/Wrapper/DirPermissionsMask.php b/lib/private/Files/Storage/Wrapper/DirPermissionsMask.php index 8d4dbbabbaae6..da4f7a90871c8 100644 --- a/lib/private/Files/Storage/Wrapper/DirPermissionsMask.php +++ b/lib/private/Files/Storage/Wrapper/DirPermissionsMask.php @@ -10,6 +10,7 @@ namespace OC\Files\Storage\Wrapper; use OC\Files\Cache\Wrapper\CacheDirPermissionsMask; +use OC\Files\Utils\PartialFileHelper; use OCP\Files\Cache\ICache; use OCP\Files\Storage\IStorage; @@ -94,15 +95,7 @@ public function getPermissions($path): int { #[\Override] public function rename($source, $target): bool { - $isPartialUploadFile = dirname($source) === dirname($target) - && strpos($source, '.ocTransferId') > 0; - - if ( - !($isPartialUploadFile - || $this->isUpdatable($source) - || $this->isDeletable($source) - ) - ) { + if (!(PartialFileHelper::isPartialFilePath($source) || $this->isUpdatable($source) || $this->isDeletable($source))) { return false; } diff --git a/lib/private/Files/Storage/Wrapper/PermissionsMask.php b/lib/private/Files/Storage/Wrapper/PermissionsMask.php index b85e8b8218326..a5757a17f8923 100644 --- a/lib/private/Files/Storage/Wrapper/PermissionsMask.php +++ b/lib/private/Files/Storage/Wrapper/PermissionsMask.php @@ -9,7 +9,7 @@ namespace OC\Files\Storage\Wrapper; use OC\Files\Cache\Wrapper\CachePermissionsMask; -use OC\Files\Storage\Storage; +use OC\Files\Utils\PartialFileHelper; use OCP\Constants; use OCP\Files\Cache\ICache; use OCP\Files\Cache\IScanner; @@ -70,13 +70,13 @@ public function getPermissions(string $path): int { #[\Override] public function rename(string $source, string $target): bool { - //This is a rename of the transfer file to the original file - $sourceDir = dirname($source); - $targetDir = dirname($target); - if (($sourceDir === $targetDir || $sourceDir === dirname($targetDir)) && strpos($source, '.ocTransferId') > 0) { - return $this->checkMask(Constants::PERMISSION_CREATE) && parent::rename($source, $target); + $required = Constants::PERMISSION_UPDATE; + if (PartialFileHelper::isPartialFilePath($source) && !$this->file_exists($target)) { + // This is a rename of a partial file to the final file + $required = Constants::PERMISSION_CREATE; } - return $this->checkMask(Constants::PERMISSION_UPDATE) && parent::rename($source, $target); + + return $this->checkMask($required) && parent::rename($source, $target); } #[\Override] diff --git a/lib/private/Files/Utils/PartialFileHelper.php b/lib/private/Files/Utils/PartialFileHelper.php new file mode 100644 index 0000000000000..d2c59bc38f65a --- /dev/null +++ b/lib/private/Files/Utils/PartialFileHelper.php @@ -0,0 +1,19 @@ +sourceStorage = new Temporary([]); + $this->instance = $this->getMaskedStorage(Constants::PERMISSION_ALL); + } + + #[\Override] + protected function tearDown(): void { + $this->sourceStorage->cleanUp(); + parent::tearDown(); + } + + protected function getMaskedStorage(int $mask): DirPermissionsMask { + return new DirPermissionsMask([ + 'storage' => $this->sourceStorage, + 'mask' => $mask, + 'path' => self::MASKED_PATH, + ]); + } + + public static function dataRenamePartFile(): array { + // Upload-only public link: no UPDATE, no DELETE + $uploadOnly = Constants::PERMISSION_READ | Constants::PERMISSION_CREATE; + $partialFileName = 'x.ocTransferId123.part'; + return [ + 'partial file in same directory' => [ + 'mask' => $uploadOnly, + 'sourcePath' => 'files/drop/' . $partialFileName, + 'targetPath' => 'files/drop/x', + 'shouldSucceed' => true, + ], + 'partial file in share root, target nested (part_file_in_storage=false)' => [ + 'mask' => $uploadOnly, + 'sourcePath' => 'files/drop/' . $partialFileName, + 'targetPath' => 'files/drop/a/b/x', + 'shouldSucceed' => true, + ], + 'partial file onto existing target without update permission' => [ + 'mask' => $uploadOnly, + 'sourcePath' => 'files/drop/' . $partialFileName, + 'targetPath' => 'files/drop/a/x', + 'shouldSucceed' => false, + 'targetExisting' => true, + ], + 'partial file onto existing target with update permission' => [ + 'mask' => $uploadOnly | Constants::PERMISSION_UPDATE, + 'sourcePath' => 'files/drop/' . $partialFileName, + 'targetPath' => 'files/drop/a/x', + 'shouldSucceed' => true, + 'targetExisting' => true, + ], + 'partial file without create permission' => [ + 'mask' => Constants::PERMISSION_READ, + 'sourcePath' => 'files/drop/' . $partialFileName, + 'targetPath' => 'files/drop/x', + 'shouldSucceed' => false, + ], + 'transfer id without .part extension' => [ + 'mask' => $uploadOnly, + 'sourcePath' => 'files/drop/foo.ocTransferId123', + 'targetPath' => 'files/drop/a/x', + 'shouldSucceed' => false, + ], + 'regular file without update or delete permission' => [ + 'mask' => $uploadOnly, + 'sourcePath' => 'files/drop/foo', + 'targetPath' => 'files/drop/x', + 'shouldSucceed' => false, + ], + 'outside masked path the mask does not apply' => [ + 'mask' => Constants::PERMISSION_READ, + 'sourcePath' => 'cache/foo', + 'targetPath' => 'cache/bar', + 'shouldSucceed' => true, + ], + ]; + } + + #[DataProvider(methodName: 'dataRenamePartFile')] + public function testRenamePartFile(int $mask, string $sourcePath, string $targetPath, bool $shouldSucceed, bool $targetExisting = false): void { + // Set up the tree on the unmasked storage so the mask doesn't interfere with the fixtures + foreach ([dirname($sourcePath), dirname($targetPath)] as $dir) { + $path = ''; + foreach (explode('/', $dir) as $part) { + $path .= $part . '/'; + if (!$this->sourceStorage->is_dir($path)) { + $this->assertTrue($this->sourceStorage->mkdir($path)); + } + } + } + $this->assertNotFalse($this->sourceStorage->file_put_contents($sourcePath, 'new')); + if ($targetExisting) { + $this->assertNotFalse($this->sourceStorage->file_put_contents($targetPath, 'existing')); + } + + $storage = $this->getMaskedStorage($mask); + + $this->assertSame($shouldSucceed, $storage->rename($sourcePath, $targetPath), 'Renaming should ' . ($shouldSucceed ? '' : 'not ') . 'succeed'); + if ($shouldSucceed) { + $this->assertFalse($this->sourceStorage->file_exists($sourcePath)); + $this->assertSame('new', $this->sourceStorage->file_get_contents($targetPath)); + } else { + $this->assertTrue($this->sourceStorage->file_exists($sourcePath)); + if ($targetExisting) { + $this->assertSame('existing', $this->sourceStorage->file_get_contents($targetPath)); + if ($shouldSucceed) { + $this->assertFalse($this->sourceStorage->file_exists($sourcePath)); + $this->assertSame('new', $this->sourceStorage->file_get_contents($targetPath)); + } else { + $this->assertTrue($this->sourceStorage->file_exists($sourcePath)); + if ($targetExisting) { + $this->assertSame('existing', $this->sourceStorage->file_get_contents($targetPath)); + } + } + } + } + } +} diff --git a/tests/lib/Files/Storage/Wrapper/PermissionsMaskTest.php b/tests/lib/Files/Storage/Wrapper/PermissionsMaskTest.php index 724ab30c73ea3..2c2954a543f81 100644 --- a/tests/lib/Files/Storage/Wrapper/PermissionsMaskTest.php +++ b/tests/lib/Files/Storage/Wrapper/PermissionsMaskTest.php @@ -13,6 +13,7 @@ use OC\Files\Storage\Wrapper\Wrapper; use OCP\Constants; use OCP\Files\Cache\IScanner; +use PHPUnit\Framework\Attributes\DataProvider; #[\PHPUnit\Framework\Attributes\Group('DB')] class PermissionsMaskTest extends \Test\Files\Storage\Storage { @@ -47,6 +48,60 @@ public function testMkdirNoCreate(): void { $this->assertFalse($storage->file_exists('foo')); } + public static function dataRenamePartFile(): array { + $partialFileName = 'x.ocTransferId123.part'; + return [ + 'renaming partial file in same directory' => [ + 'sourcePath' => $partialFileName, + 'targetPath' => 'x', + 'shouldSucceed' => true, + ], + 'renaming partial file for file uploaded in parent (part_file_in_storage=false) ' => [ + 'sourcePath' => '/A/' . $partialFileName, + 'targetPath' => '/A/Share/x', + 'shouldSucceed' => true, + ], + 'renaming partial file for file uploaded in ancestor (part_file_in_storage=false) ' => [ + 'sourcePath' => '/A/' . $partialFileName, + 'targetPath' => '/A/B/C/Share/x', + 'shouldSucceed' => true, + ], + 'renaming non-partial file in parent' => [ + 'sourcePath' => '/A/foo.ocTransferId123', + 'targetPath' => '/A/Share/x', + 'shouldSucceed' => false, + ], + 'renaming partial file into existing file' => [ + 'sourcePath' => '/A/' . $partialFileName, + 'targetPath' => '/A/Share/x', + 'shouldSucceed' => false, + 'targetExisting' => true, + ] + ]; + } + #[DataProvider(methodName: 'dataRenamePartFile')] + public function testRenamePartFile(string $sourcePath, string $targetPath, bool $shouldSucceed, bool $targetExisting = false): void { + $storage = $this->getMaskedStorage(Constants::PERMISSION_ALL - Constants::PERMISSION_UPDATE); + $directories = array_unique([ ...explode('/', dirname($sourcePath)), ...explode('/', dirname($targetPath)) ]); + $path = ''; + foreach ($directories as $part) { + if (in_array($part, ['', '.'], true)) { + continue; + } + $path .= $part . '/'; + $this->assertTrue($storage->mkdir($path)); + } + + $this->assertNotFalse($this->sourceStorage->file_put_contents($sourcePath, 'bar')); + $this->assertTrue($this->sourceStorage->file_exists($sourcePath)); + if ($targetExisting) { + $this->assertNotFalse($this->sourceStorage->file_put_contents($targetPath, 'existing')); + $this->assertTrue($this->sourceStorage->file_exists($targetPath)); + } + + $this->assertEquals($shouldSucceed, $storage->rename($sourcePath, $targetPath), 'Renaming should ' . ($shouldSucceed ? '' : 'not ') . 'succeed'); + } + public function testRmdirNoDelete(): void { $storage = $this->getMaskedStorage(Constants::PERMISSION_ALL - Constants::PERMISSION_DELETE); $this->assertTrue($storage->mkdir('foo')); From fd8fc6bc0c8ac8ff5adcc6bb205baa7a972d2215 Mon Sep 17 00:00:00 2001 From: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> Date: Wed, 14 Oct 2026 16:09:33 +0200 Subject: [PATCH 2/2] fix: pass missing mtime in touch Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> --- .../Storage/Wrapper/DirPermissionsMask.php | 7 +++---- .../Files/Storage/Wrapper/PermissionsMask.php | 12 +++++++++--- lib/private/Files/Utils/PartialFileHelper.php | 19 ------------------- 3 files changed, 12 insertions(+), 26 deletions(-) delete mode 100644 lib/private/Files/Utils/PartialFileHelper.php diff --git a/lib/private/Files/Storage/Wrapper/DirPermissionsMask.php b/lib/private/Files/Storage/Wrapper/DirPermissionsMask.php index da4f7a90871c8..492cdd8e172b5 100644 --- a/lib/private/Files/Storage/Wrapper/DirPermissionsMask.php +++ b/lib/private/Files/Storage/Wrapper/DirPermissionsMask.php @@ -10,7 +10,6 @@ namespace OC\Files\Storage\Wrapper; use OC\Files\Cache\Wrapper\CacheDirPermissionsMask; -use OC\Files\Utils\PartialFileHelper; use OCP\Files\Cache\ICache; use OCP\Files\Storage\IStorage; @@ -95,7 +94,7 @@ public function getPermissions($path): int { #[\Override] public function rename($source, $target): bool { - if (!(PartialFileHelper::isPartialFilePath($source) || $this->isUpdatable($source) || $this->isDeletable($source))) { + if (!($this->isPartialFileMove($source, $target) || $this->isUpdatable($source) || $this->isDeletable($source))) { return false; } @@ -144,10 +143,10 @@ public function copy($source, $target): bool { #[\Override] public function touch($path, $mtime = null): bool { if ($this->checkPath($path)) { - return parent::touch($path); + return parent::touch($path, $mtime); } - return $this->storage->touch($path); + return $this->storage->touch($path, $mtime); } #[\Override] diff --git a/lib/private/Files/Storage/Wrapper/PermissionsMask.php b/lib/private/Files/Storage/Wrapper/PermissionsMask.php index a5757a17f8923..8ae0fcf4caef0 100644 --- a/lib/private/Files/Storage/Wrapper/PermissionsMask.php +++ b/lib/private/Files/Storage/Wrapper/PermissionsMask.php @@ -9,7 +9,6 @@ namespace OC\Files\Storage\Wrapper; use OC\Files\Cache\Wrapper\CachePermissionsMask; -use OC\Files\Utils\PartialFileHelper; use OCP\Constants; use OCP\Files\Cache\ICache; use OCP\Files\Cache\IScanner; @@ -71,8 +70,8 @@ public function getPermissions(string $path): int { #[\Override] public function rename(string $source, string $target): bool { $required = Constants::PERMISSION_UPDATE; - if (PartialFileHelper::isPartialFilePath($source) && !$this->file_exists($target)) { - // This is a rename of a partial file to the final file + if ($this->isPartialFileMove($source, $target) && !$this->file_exists($target)) { + // This is a move/rename of a partial file to the final file $required = Constants::PERMISSION_CREATE; } @@ -158,4 +157,11 @@ public function getDirectoryContent(string $directory): \Traversable { yield $data; } } + + /** + * If source and target are compatible with a rename/move operation of a partial file into its final location + */ + protected function isPartialFileMove(string $source, string $target): bool { + return str_starts_with(dirname($target), dirname($source)) && preg_match('/\.ocTransferId\d+\.part$/i', $source) === 1; + } } diff --git a/lib/private/Files/Utils/PartialFileHelper.php b/lib/private/Files/Utils/PartialFileHelper.php deleted file mode 100644 index d2c59bc38f65a..0000000000000 --- a/lib/private/Files/Utils/PartialFileHelper.php +++ /dev/null @@ -1,19 +0,0 @@ -