From c567b1d6b221ea5ffd48c88b17e404d85f2bb19a Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 29 Jun 2020 18:14:47 +0200 Subject: [PATCH 1/6] 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 a85f761abb..c027f1cae7 100644 --- a/apps/files_trashbin/lib/Trashbin.php +++ b/apps/files_trashbin/lib/Trashbin.php @@ -278,6 +278,8 @@ class Trashbin { /** @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 @@ class Trashbin { 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 93e13937f5..a629795181 100644 --- a/lib/private/Files/Storage/Common.php +++ b/lib/private/Files/Storage/Common.php @@ -620,18 +620,15 @@ abstract class Common implements Storage, ILockingStorage, IWriteStreamStorage { } } 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 b6a600e6535fb972e1e4c5c33dd6bd5cf7a097c2 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 30 Jun 2020 16:09:50 +0200 Subject: [PATCH 2/6] 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 c027f1cae7..ca5b1be24f 100644 --- a/apps/files_trashbin/lib/Trashbin.php +++ b/apps/files_trashbin/lib/Trashbin.php @@ -278,6 +278,8 @@ class Trashbin { /** @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 @@ class Trashbin { } 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 9dbe2a4e10..acaa5fc98b 100644 --- a/tests/lib/Files/ViewTest.php +++ b/tests/lib/Files/ViewTest.php @@ -1167,13 +1167,8 @@ class ViewTest extends \Test\TestCase { ->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 7ac4d57fd5d3863a0ef0325abfa58a9f7e5020a1 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 1 Jul 2020 15:37:47 +0200 Subject: [PATCH 3/6] 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 a629795181..2d03598141 100644 --- a/lib/private/Files/Storage/Common.php +++ b/lib/private/Files/Storage/Common.php @@ -53,6 +53,7 @@ use OC\Files\Storage\Wrapper\Jail; 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 @@ abstract class Common implements Storage, ILockingStorage, IWriteStreamStorage { } } 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 @@ abstract class Common implements Storage, ILockingStorage, IWriteStreamStorage { 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 4cf3ac4799..0b636d06bd 100644 --- a/lib/private/Files/Storage/Local.php +++ b/lib/private/Files/Storage/Local.php @@ -44,6 +44,7 @@ use OC\Files\Filesystem; 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 @@ class Local extends \OC\Files\Storage\Common { } 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 acaa5fc98b..e69e49f8c8 100644 --- a/tests/lib/Files/ViewTest.php +++ b/tests/lib/Files/ViewTest.php @@ -13,7 +13,6 @@ use OC\Files\Filesystem; 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 142b6eb08f5e274c80943ae6d70922bba2f25e83 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 23 Jul 2020 20:31:35 +0200 Subject: [PATCH 4/6] 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 60534a153d..b6a0e62a88 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 @@ class Updater implements IUpdater { $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 @@ class Updater implements IUpdater { } 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) { From 4e6719b28c5bc5f4367af128e1ed731a1f3e6c4d Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 30 Jul 2020 16:11:45 +0200 Subject: [PATCH 5/6] dont update mimetype when moving to trash Signed-off-by: Robin Appelman --- lib/private/Files/Cache/Updater.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/private/Files/Cache/Updater.php b/lib/private/Files/Cache/Updater.php index b6a0e62a88..79501d910e 100644 --- a/lib/private/Files/Cache/Updater.php +++ b/lib/private/Files/Cache/Updater.php @@ -201,7 +201,11 @@ class Updater implements IUpdater { $this->cache->moveFromCache($sourceCache, $source, $target); } - if (pathinfo($source, PATHINFO_EXTENSION) !== pathinfo($target, PATHINFO_EXTENSION) && $sourceInfo->getMimeType() !== FileInfo::MIMETYPE_FOLDER) { + $sourceExtension = pathinfo($source, PATHINFO_EXTENSION); + $targetExtension = pathinfo($target, PATHINFO_EXTENSION); + $targetIsTrash = preg_match("/d\d+/", $targetExtension); + + if ($sourceExtension !== $targetExtension && $sourceInfo->getMimeType() !== FileInfo::MIMETYPE_FOLDER && !$targetIsTrash) { // handle mime type change $mimeType = $this->storage->getMimeType($target); $fileId = $this->cache->getId($target); From 193b3ead51fea5706d3ef14206684286b1b75c0e Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 30 Jul 2020 16:31:56 +0200 Subject: [PATCH 6/6] fix object store trashbin handling object stores are "special" given how they interact with the cache on their own Signed-off-by: Robin Appelman --- apps/files_trashbin/lib/Trashbin.php | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/apps/files_trashbin/lib/Trashbin.php b/apps/files_trashbin/lib/Trashbin.php index ca5b1be24f..76f8867c25 100644 --- a/apps/files_trashbin/lib/Trashbin.php +++ b/apps/files_trashbin/lib/Trashbin.php @@ -44,6 +44,7 @@ namespace OCA\Files_Trashbin; use OC\Files\Filesystem; +use OC\Files\ObjectStore\ObjectStoreStorage; use OC\Files\View; use OCA\Files_Trashbin\AppInfo\Application; use OCA\Files_Trashbin\Command\Expire; @@ -278,16 +279,22 @@ class Trashbin { /** @var \OC\Files\Storage\Storage $sourceStorage */ [$sourceStorage, $sourceInternalPath] = $ownerView->resolvePath('/files/' . $ownerPath); + + if ($trashStorage->file_exists($trashInternalPath)) { + $trashStorage->unlink($trashInternalPath); + } + $connection = \OC::$server->getDatabaseConnection(); $connection->beginTransaction(); $trashStorage->getUpdater()->renameFromStorage($sourceStorage, $sourceInternalPath, $trashInternalPath); try { $moveSuccessful = true; - if ($trashStorage->file_exists($trashInternalPath)) { - $trashStorage->unlink($trashInternalPath); + + // when moving within the same object store, the cache update done above is enough to move the file + if (!($trashStorage->instanceOfStorage(ObjectStoreStorage::class) && $trashStorage->getId() === $sourceStorage->getId())) { + $trashStorage->moveFromStorage($sourceStorage, $sourceInternalPath, $trashInternalPath); } - $trashStorage->moveFromStorage($sourceStorage, $sourceInternalPath, $trashInternalPath); } catch (\OCA\Files_Trashbin\Exceptions\CopyRecursiveException $e) { $moveSuccessful = false; if ($trashStorage->file_exists($trashInternalPath)) {