From 59498493220a666573516679e7752d09543ac1b5 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 29 Jun 2020 18:14:47 +0200 Subject: [PATCH 1/4] fix moving files from external storage to object store trashbin having the "cache rename" after the "storage move" caused the target to get the fileid from the source file, without taking care that the object is stored under the original file id. By doing the "cache rename" first, we trigger the "update existing file" logic while moving the file to the object store and the object gets stored for the correct file id Signed-off-by: Robin Appelman --- apps/files_trashbin/lib/Trashbin.php | 4 ++-- lib/private/Files/Storage/Common.php | 15 ++++++--------- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/apps/files_trashbin/lib/Trashbin.php b/apps/files_trashbin/lib/Trashbin.php index f73cc1f4aa6af..e23368f825bee 100644 --- a/apps/files_trashbin/lib/Trashbin.php +++ b/apps/files_trashbin/lib/Trashbin.php @@ -278,6 +278,8 @@ public static function move2trash($file_path, $ownerOnly = false) { /** @var \OC\Files\Storage\Storage $sourceStorage */ [$sourceStorage, $sourceInternalPath] = $ownerView->resolvePath('/files/' . $ownerPath); + $trashStorage->getUpdater()->renameFromStorage($sourceStorage, $sourceInternalPath, $trashInternalPath); + try { $moveSuccessful = true; if ($trashStorage->file_exists($trashInternalPath)) { @@ -301,8 +303,6 @@ public static function move2trash($file_path, $ownerOnly = false) { return false; } - $trashStorage->getUpdater()->renameFromStorage($sourceStorage, $sourceInternalPath, $trashInternalPath); - if ($moveSuccessful) { $query = \OC_DB::prepare("INSERT INTO `*PREFIX*files_trash` (`id`,`timestamp`,`location`,`user`) VALUES (?,?,?,?)"); $result = $query->execute([$filename, $timestamp, $location, $owner]); diff --git a/lib/private/Files/Storage/Common.php b/lib/private/Files/Storage/Common.php index a62b7d727fb0a..ada037768bd09 100644 --- a/lib/private/Files/Storage/Common.php +++ b/lib/private/Files/Storage/Common.php @@ -620,18 +620,15 @@ public function copyFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t } } else { $source = $sourceStorage->fopen($sourceInternalPath, 'r'); - // TODO: call fopen in a way that we execute again all storage wrappers - // to avoid that we bypass storage wrappers which perform important actions - // for this operation. Same is true for all other operations which - // are not the same as the original one.Once this is fixed we also - // need to adjust the encryption wrapper. - $target = $this->fopen($targetInternalPath, 'w'); - [, $result] = \OC_Helper::streamCopy($source, $target); + if ($source) { + $result = $this->writeStream($targetInternalPath, $source) > 0; + } else { + $result = false; + } + if ($result and $preserveMtime) { $this->touch($targetInternalPath, $sourceStorage->filemtime($sourceInternalPath)); } - fclose($source); - fclose($target); if (!$result) { // delete partially written target file From fcad692b4a5dd8e0c128af64647b64f658b124c5 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 30 Jun 2020 16:09:50 +0200 Subject: [PATCH 2/4] rollback cache rename if trashbin move fails Signed-off-by: Robin Appelman --- apps/files_trashbin/lib/Trashbin.php | 5 +++++ tests/lib/Files/ViewTest.php | 9 ++------- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/apps/files_trashbin/lib/Trashbin.php b/apps/files_trashbin/lib/Trashbin.php index e23368f825bee..db00a7ed27288 100644 --- a/apps/files_trashbin/lib/Trashbin.php +++ b/apps/files_trashbin/lib/Trashbin.php @@ -278,6 +278,8 @@ public static function move2trash($file_path, $ownerOnly = false) { /** @var \OC\Files\Storage\Storage $sourceStorage */ [$sourceStorage, $sourceInternalPath] = $ownerView->resolvePath('/files/' . $ownerPath); + $connection = \OC::$server->getDatabaseConnection(); + $connection->beginTransaction(); $trashStorage->getUpdater()->renameFromStorage($sourceStorage, $sourceInternalPath, $trashInternalPath); try { @@ -300,9 +302,12 @@ public static function move2trash($file_path, $ownerOnly = false) { } else { $sourceStorage->unlink($sourceInternalPath); } + $connection->rollBack(); return false; } + $connection->commit(); + if ($moveSuccessful) { $query = \OC_DB::prepare("INSERT INTO `*PREFIX*files_trash` (`id`,`timestamp`,`location`,`user`) VALUES (?,?,?,?)"); $result = $query->execute([$filename, $timestamp, $location, $owner]); diff --git a/tests/lib/Files/ViewTest.php b/tests/lib/Files/ViewTest.php index 036fc038a600e..ae6c4b22dec56 100644 --- a/tests/lib/Files/ViewTest.php +++ b/tests/lib/Files/ViewTest.php @@ -1167,13 +1167,8 @@ private function doTestCopyRenameFail($operation) { ->setMethods(['fopen']) ->getMock(); - $storage2->expects($this->any()) - ->method('fopen') - ->willReturnCallback(function ($path, $mode) use ($storage2) { - /** @var \PHPUnit_Framework_MockObject_MockObject | \OC\Files\Storage\Temporary $storage2 */ - $source = fopen($storage2->getSourcePath($path), $mode); - return Quota::wrap($source, 9); - }); + $storage2->method('writeStream') + ->willReturn(0); $storage1->mkdir('sub'); $storage1->file_put_contents('foo.txt', '0123456789ABCDEFGH'); From ad7798f9c9066b1eee6ad91526ffab34186f4a7b Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 1 Jul 2020 15:37:47 +0200 Subject: [PATCH 3/4] use exceptions for error signaling in writeStream this remove the ambiguity when writing zero length files Signed-off-by: Robin Appelman --- lib/private/Files/Storage/Common.php | 16 ++++++++++++---- lib/private/Files/Storage/Local.php | 8 +++++++- tests/lib/Files/ViewTest.php | 1 - 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/lib/private/Files/Storage/Common.php b/lib/private/Files/Storage/Common.php index ada037768bd09..958d09832c4b0 100644 --- a/lib/private/Files/Storage/Common.php +++ b/lib/private/Files/Storage/Common.php @@ -53,6 +53,7 @@ use OC\Files\Storage\Wrapper\Wrapper; use OCP\Files\EmptyFileNameException; use OCP\Files\FileNameTooLongException; +use OCP\Files\GenericFileException; use OCP\Files\InvalidCharacterInPathException; use OCP\Files\InvalidDirectoryException; use OCP\Files\InvalidPathException; @@ -620,10 +621,14 @@ public function copyFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t } } else { $source = $sourceStorage->fopen($sourceInternalPath, 'r'); + $result = false; if ($source) { - $result = $this->writeStream($targetInternalPath, $source) > 0; - } else { - $result = false; + try { + $this->writeStream($targetInternalPath, $source); + $result = true; + } catch (\Exception $e) { + \OC::$server->getLogger()->logException($e, ['level' => ILogger::WARN, 'message' => 'Failed to copy stream to storage']); + } } if ($result and $preserveMtime) { @@ -855,10 +860,13 @@ public function needsPartFile() { public function writeStream(string $path, $stream, int $size = null): int { $target = $this->fopen($path, 'w'); if (!$target) { - return 0; + throw new GenericFileException("Failed to open $path for writing"); } try { [$count, $result] = \OC_Helper::streamCopy($stream, $target); + if (!$result) { + throw new GenericFileException("Failed to copy stream"); + } } finally { fclose($target); fclose($stream); diff --git a/lib/private/Files/Storage/Local.php b/lib/private/Files/Storage/Local.php index 4cf3ac4799fa5..0b636d06bde3d 100644 --- a/lib/private/Files/Storage/Local.php +++ b/lib/private/Files/Storage/Local.php @@ -44,6 +44,7 @@ use OC\Files\Storage\Wrapper\Jail; use OCP\Constants; use OCP\Files\ForbiddenException; +use OCP\Files\GenericFileException; use OCP\Files\Storage\IStorage; use OCP\ILogger; @@ -553,6 +554,11 @@ public function moveFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t } public function writeStream(string $path, $stream, int $size = null): int { - return (int)file_put_contents($this->getSourcePath($path), $stream); + $result = file_put_contents($this->getSourcePath($path), $stream); + if ($result === false) { + throw new GenericFileException("Failed write steam to $path"); + } else { + return $result; + } } } diff --git a/tests/lib/Files/ViewTest.php b/tests/lib/Files/ViewTest.php index ae6c4b22dec56..fad2217bfdbed 100644 --- a/tests/lib/Files/ViewTest.php +++ b/tests/lib/Files/ViewTest.php @@ -13,7 +13,6 @@ use OC\Files\Mount\MountPoint; use OC\Files\Storage\Common; use OC\Files\Storage\Temporary; -use OC\Files\Stream\Quota; use OC\Files\View; use OCP\Constants; use OCP\Files\Config\IMountProvider; From c8cf2e8a5bc6b093cbcd06e93d7599fafe641c06 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 23 Jul 2020 20:31:35 +0200 Subject: [PATCH 4/4] fix renameFromStorage messing with folder mimetype Signed-off-by: Robin Appelman --- lib/private/Files/Cache/Updater.php | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/lib/private/Files/Cache/Updater.php b/lib/private/Files/Cache/Updater.php index 60534a153dd00..b6a0e62a88a77 100644 --- a/lib/private/Files/Cache/Updater.php +++ b/lib/private/Files/Cache/Updater.php @@ -28,6 +28,7 @@ namespace OC\Files\Cache; +use OC\Files\FileInfo; use OCP\Files\Cache\ICacheEntry; use OCP\Files\Cache\IUpdater; use OCP\Files\Storage\IStorage; @@ -187,7 +188,9 @@ public function renameFromStorage(IStorage $sourceStorage, $source, $target) { $sourceUpdater = $sourceStorage->getUpdater(); $sourcePropagator = $sourceStorage->getPropagator(); - if ($sourceCache->inCache($source)) { + $sourceInfo = $sourceCache->get($source); + + if ($sourceInfo !== false) { if ($this->cache->inCache($target)) { $this->cache->remove($target); } @@ -197,13 +200,13 @@ public function renameFromStorage(IStorage $sourceStorage, $source, $target) { } else { $this->cache->moveFromCache($sourceCache, $source, $target); } - } - if (pathinfo($source, PATHINFO_EXTENSION) !== pathinfo($target, PATHINFO_EXTENSION)) { - // handle mime type change - $mimeType = $this->storage->getMimeType($target); - $fileId = $this->cache->getId($target); - $this->cache->update($fileId, ['mimetype' => $mimeType]); + if (pathinfo($source, PATHINFO_EXTENSION) !== pathinfo($target, PATHINFO_EXTENSION) && $sourceInfo->getMimeType() !== FileInfo::MIMETYPE_FOLDER) { + // handle mime type change + $mimeType = $this->storage->getMimeType($target); + $fileId = $this->cache->getId($target); + $this->cache->update($fileId, ['mimetype' => $mimeType]); + } } if ($sourceCache instanceof Cache) {