diff --git a/lib/private/Files/Storage/Wrapper/DirPermissionsMask.php b/lib/private/Files/Storage/Wrapper/DirPermissionsMask.php index e1066e8d04f20..3949660cc9d5a 100644 --- a/lib/private/Files/Storage/Wrapper/DirPermissionsMask.php +++ b/lib/private/Files/Storage/Wrapper/DirPermissionsMask.php @@ -88,15 +88,7 @@ public function getPermissions($path): int { } public function rename($source, $target): bool { - $isPartialUploadFile = dirname($source) === dirname($target) - && strpos($source, '.ocTransferId') > 0; - - if ( - !($isPartialUploadFile - || $this->isUpdatable($source) - || $this->isDeletable($source) - ) - ) { + if (!($this->isPartialFileMove($source, $target) || $this->isUpdatable($source) || $this->isDeletable($source))) { return false; } @@ -143,10 +135,10 @@ public function copy($source, $target): bool { 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); } public function mkdir($path): bool { diff --git a/lib/private/Files/Storage/Wrapper/PermissionsMask.php b/lib/private/Files/Storage/Wrapper/PermissionsMask.php index a32986cda1407..1a9b340b6d087 100644 --- a/lib/private/Files/Storage/Wrapper/PermissionsMask.php +++ b/lib/private/Files/Storage/Wrapper/PermissionsMask.php @@ -5,10 +5,10 @@ * SPDX-FileCopyrightText: 2016 ownCloud, Inc. * SPDX-License-Identifier: AGPL-3.0-only */ + namespace OC\Files\Storage\Wrapper; use OC\Files\Cache\Wrapper\CachePermissionsMask; -use OC\Files\Storage\Storage; use OCP\Constants; use OCP\Files\Storage\IStorage; @@ -61,13 +61,13 @@ public function getPermissions(string $path): int { } 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 ($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; } - return $this->checkMask(Constants::PERMISSION_UPDATE) && parent::rename($source, $target); + + return $this->checkMask($required) && parent::rename($source, $target); } public function copy(string $source, string $target): bool { @@ -138,4 +138,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/tests/lib/Files/Storage/Wrapper/DirPermissionsMaskTest.php b/tests/lib/Files/Storage/Wrapper/DirPermissionsMaskTest.php new file mode 100644 index 0000000000000..7efa66f920044 --- /dev/null +++ b/tests/lib/Files/Storage/Wrapper/DirPermissionsMaskTest.php @@ -0,0 +1,142 @@ +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 1b4a55b010b67..7a7f867d4b87e 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 { @@ -45,6 +46,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'));