From 7e12f9c9bb757fa71abd216b21223cc738b8acc2 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 972afe40dcd..2647cb2f0f8 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(array($filename, $timestamp, $location, $owner)); diff --git a/lib/private/Files/Storage/Common.php b/lib/private/Files/Storage/Common.php index 82ac49b9fdf..10941937a55 100644 --- a/lib/private/Files/Storage/Common.php +++ b/lib/private/Files/Storage/Common.php @@ -618,18 +618,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'); - list(, $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 e860507201dc68260dc8633046ab053a93586146 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 | 13 ++++--------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/apps/files_trashbin/lib/Trashbin.php b/apps/files_trashbin/lib/Trashbin.php index 2647cb2f0f8..9eccebe64df 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(array($filename, $timestamp, $location, $owner)); diff --git a/tests/lib/Files/ViewTest.php b/tests/lib/Files/ViewTest.php index 57050059375..e394e0a8651 100644 --- a/tests/lib/Files/ViewTest.php +++ b/tests/lib/Files/ViewTest.php @@ -242,7 +242,7 @@ class ViewTest extends \Test\TestCase { $this->assertEquals('/foo.txt', $folderView->getPath($id2)); } - + public function testGetPathNotExisting() { $this->expectException(\OCP\Files\NotFoundException::class); @@ -1167,13 +1167,8 @@ class ViewTest extends \Test\TestCase { ->setMethods(['fopen']) ->getMock(); - $storage2->expects($this->any()) - ->method('fopen') - ->will($this->returnCallback(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'); @@ -1291,7 +1286,7 @@ class ViewTest extends \Test\TestCase { $this->assertNull($view->getRelativePath(null)); } - + public function testNullAsRoot() { $this->expectException(\InvalidArgumentException::class); From 98ca765572568640f17f917a0877944d0b9f57cb 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 10941937a55..403864bbb1e 100644 --- a/lib/private/Files/Storage/Common.php +++ b/lib/private/Files/Storage/Common.php @@ -52,6 +52,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; @@ -618,10 +619,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) { @@ -852,10 +857,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 13a3ca87097..5f86c5b75f8 100644 --- a/lib/private/Files/Storage/Local.php +++ b/lib/private/Files/Storage/Local.php @@ -43,6 +43,7 @@ namespace OC\Files\Storage; use OC\Files\Filesystem; use OC\Files\Storage\Wrapper\Jail; use OCP\Files\ForbiddenException; +use OCP\Files\GenericFileException; use OCP\Files\Storage\IStorage; use OCP\ILogger; @@ -500,6 +501,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 e394e0a8651..ea35b560877 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\Files\Config\IMountProvider; use OCP\Files\FileInfo; From c3f2b73a97dc8a349caf9b0d9f1d0d328239d975 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 59cff9b3a41..7c222b31032 100644 --- a/lib/private/Files/Cache/Updater.php +++ b/lib/private/Files/Cache/Updater.php @@ -27,6 +27,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 ae6357ca6e6583d09f13affa31fca54c5e747a4a 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 7c222b31032..b5d8fd0054f 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 8d9c41c3477e97e40a0ab23423cf1106192cb3a6 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 9eccebe64df..27242a972d1 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)) {