From b6b80fd7da46995ba945b656c4482dfe2493cf99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Pablo=20Villaf=C3=A1=C3=B1ez?= Date: Thu, 17 Mar 2022 10:24:09 +0100 Subject: [PATCH 1/4] Improve quota handling * In case there are problems writing the contents of the file, try to figure out the size of the content in several ways in order to provide a more meaningful message * Ensure the locks are properly released in case an error happens * Handle quota while copying files. In addition, take into account the space used by the file we're trying to overwrite (in case there is already a file there) * Part files will also be considered for quota while they're being uploaded --- apps/dav/lib/Connector/Sabre/File.php | 19 ++++++++ apps/dav/lib/Connector/Sabre/QuotaPlugin.php | 48 ++++++++++++++++++-- lib/private/Files/Storage/Wrapper/Quota.php | 43 ++++++++++++++---- 3 files changed, 98 insertions(+), 12 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php index 5303651315cd..3f4fd329e1fd 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -223,6 +223,13 @@ public function put($data) { $expected = -1; if (isset($_SERVER['CONTENT_LENGTH'])) { $expected = $_SERVER['CONTENT_LENGTH']; + } elseif (\is_resource($data)) { + $stat = \fstat($data); + if ($stat !== false) { + $expected = $stat['size']; + } + } elseif (\is_string($data)) { + $expected = \strlen($data); } throw new Exception('Error while copying file to target location (copied bytes: ' . $count . ', expected filesize: ' . $expected . ' )'); } @@ -276,9 +283,21 @@ public function put($data) { throw new Exception('Could not rename part file to final file'); } } catch (ForbiddenException $ex) { + // try to revert the lock state so it can be cleanup properly + try { + $this->changeLock(ILockingProvider::LOCK_SHARED); + } catch (LockedException $e) { + $this->convertToSabreException($e); + } throw new DAVForbiddenException($ex->getMessage(), $ex->getRetry()); } catch (\Exception $e) { $partStorage->unlink($internalPartPath); + // try to revert the lock state so it can be cleanup properly + try { + $this->changeLock(ILockingProvider::LOCK_SHARED); + } catch (LockedException $e) { + $this->convertToSabreException($e); + } $this->convertToSabreException($e); } } diff --git a/apps/dav/lib/Connector/Sabre/QuotaPlugin.php b/apps/dav/lib/Connector/Sabre/QuotaPlugin.php index 4de12956b798..15c273293b0b 100644 --- a/apps/dav/lib/Connector/Sabre/QuotaPlugin.php +++ b/apps/dav/lib/Connector/Sabre/QuotaPlugin.php @@ -29,6 +29,7 @@ use Sabre\DAV\Exception\InsufficientStorage; use Sabre\DAV\Exception\ServiceUnavailable; use Sabre\DAV\INode; +use Sabre\HTTP\RequestInterface; /** * This plugin check user quota and deny creating files when they exceeds the quota. @@ -75,6 +76,15 @@ public function initialize(\Sabre\DAV\Server $server) { $server->on('beforeWriteContent', [$this, 'handleBeforeWriteContent'], 10); $server->on('beforeCreateFile', [$this, 'handleBeforeCreateFile'], 10); $server->on('beforeMove', [$this, 'handleBeforeMove'], 10); + + // QuotaPlugin is initialized during the processing of the "beforeMethod:*" event handling, which + // means that we can't listen on a "beforeMethod:*" event at this point because it's too late + // There is no "beforeCopy" event we can listen to neither. + // We'll assume the "beforeMethod:*" is being processed, so we'll process our "handleBeforeCopy" + // right away. + if ($server->httpRequest->getMethod() === 'COPY') { + $this->handleBeforeCopy($server->httpRequest); + } } /** @@ -90,8 +100,12 @@ public function handleBeforeMove($source, $destination) { $sourceNode = $this->server->tree->getNodeForPath($source); // get target node for proper path conversion + $destinationSize = 0; if ($this->server->tree->nodeExists($destination)) { $targetNode = $this->server->tree->getNodeForPath($destination); + if ($targetNode instanceof Node) { + $destinationSize = $targetNode->getSize() ?? 0; + } } else { $dirname = \dirname($destination); $dirname = $dirname === '.' ? '/' : $dirname; @@ -112,7 +126,30 @@ public function handleBeforeMove($source, $destination) { } } $path = $targetNode->getPath(); - return $this->checkQuota($path, $sourceNode->getSize()); + return $this->checkQuota($path, $sourceNode->getSize(), $destinationSize); + } + + public function handleBeforeCopy(RequestInterface $request) { + if ($request->getMethod() !== 'COPY') { + return; + } + + $requestedPath = $request->getPath(); + $sourceNode = $this->server->tree->getNodeForPath($requestedPath); // might throw a NotFound exception + + $sourceSize = null; + if ($sourceNode instanceof Node) { + $sourceSize = $sourceNode->getSize(); + } + + $copyInfo = $this->server->getCopyAndMoveInfo($request); // it might throw exceptions + $path = $copyInfo['destination']; + + $destinationSize = 0; + if ($copyInfo['destinationExists'] && $copyInfo['destinationNode'] instanceof Node) { + $destinationSize = $copyInfo['destinationNode']->getSize() ?? 0; + } + return $this->checkQuota($path, $sourceSize, $destinationSize); } /** @@ -127,7 +164,9 @@ public function handleBeforeWriteContent($uri, $node, $data, $modified) { if (!$node instanceof Node) { return; } - return $this->checkQuota($node->getPath()); + + $nodeSize = $node->getSize() ?? 0; // it might return null, so assume 0 in that case. + return $this->checkQuota($node->getPath(), null, $nodeSize); } /** @@ -150,10 +189,11 @@ public function handleBeforeCreateFile($uri, $data, $parent, $modified) { * * @param string $path path of the user's home * @param int $length size to check whether it fits + * @param int $extraSpace additional space granted, usually used when overwriting files * @throws InsufficientStorage * @return bool */ - public function checkQuota($path, $length = null) { + public function checkQuota($path, $length = null, $extraSpace = 0) { if ($length === null) { $length = $this->getLength(); } @@ -173,7 +213,7 @@ public function checkQuota($path, $length = null) { $path = \rtrim($parentPath, '/') . '/' . $info['name']; } $freeSpace = $this->getFreeSpace($path); - if ($freeSpace !== FileInfo::SPACE_UNKNOWN && $freeSpace !== FileInfo::SPACE_UNLIMITED && $length > $freeSpace) { + if ($freeSpace !== FileInfo::SPACE_UNKNOWN && $freeSpace !== FileInfo::SPACE_UNLIMITED && $length > $freeSpace + $extraSpace) { if (isset($chunkHandler)) { $chunkHandler->cleanup(); } diff --git a/lib/private/Files/Storage/Wrapper/Quota.php b/lib/private/Files/Storage/Wrapper/Quota.php index 0ca7910c1d66..cbb0e25021d8 100644 --- a/lib/private/Files/Storage/Wrapper/Quota.php +++ b/lib/private/Files/Storage/Wrapper/Quota.php @@ -142,14 +142,22 @@ public function copy($source, $target) { public function fopen($path, $mode) { $source = $this->storage->fopen($path, $mode); - // don't apply quota for part files - if (!$this->isPartFile($path)) { - $free = $this->free_space(''); - if ($source && $free >= 0 && $mode !== 'r' && $mode !== 'rb') { - // only apply quota for files, not metadata, trash or others - if (\strpos(\ltrim($path, '/'), 'files/') === 0) { - return \OC\Files\Stream\Quota::wrap($source, $free); - } + // if it's a .part file, check if we're trying to overwrite a file + $used = \OCP\Files\FileInfo::SPACE_NOT_COMPUTED; + if ($this->isPartFile($path)) { + $used = $this->getSize($this->stripPartialFileExtension($path)); + } + + $free = $this->free_space(''); + if ($used >= 0) { + // if we're overwriting a file, add the size of that file to the available space + // so it's possible to overwrite in case the quota is limited + $free += $used; + } + if ($source && $free >= 0 && $mode !== 'r' && $mode !== 'rb') { + // only apply quota for files, not metadata, trash or others + if (\strpos(\ltrim($path, '/'), 'files/') === 0) { + return \OC\Files\Stream\Quota::wrap($source, $free); } } return $source; @@ -168,6 +176,25 @@ private function isPartFile($path) { return ($extension === 'part'); } + private function stripPartialFileExtension($path) { + $extension = \pathinfo($path, PATHINFO_EXTENSION); + + if ($extension === 'part') { + $newLength = \strlen($path) - 5; // 5 = strlen(".part") + $fPath = \substr($path, 0, $newLength); + + // if path also contains a transaction id, we remove it too + $extension = \pathinfo($fPath, PATHINFO_EXTENSION); + if (\substr($extension, 0, 12) === 'ocTransferId') { // 12 = strlen("ocTransferId") + $newLength = \strlen($fPath) - \strlen($extension) -1; + $fPath = \substr($fPath, 0, $newLength); + } + return $fPath; + } else { + return $path; + } + } + /** * @param \OCP\Files\Storage $sourceStorage * @param string $sourceInternalPath From fa57dd10dd20a956f3b3aa0dbb9a519bd1838151 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Pablo=20Villaf=C3=A1=C3=B1ez?= Date: Fri, 18 Mar 2022 12:56:58 +0100 Subject: [PATCH 2/4] Adjust quota checks for shared files --- lib/private/Files/Storage/Wrapper/Quota.php | 19 +++++++++++++++++-- lib/private/legacy/util.php | 7 ++++++- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/lib/private/Files/Storage/Wrapper/Quota.php b/lib/private/Files/Storage/Wrapper/Quota.php index cbb0e25021d8..ed59bfd3d2cd 100644 --- a/lib/private/Files/Storage/Wrapper/Quota.php +++ b/lib/private/Files/Storage/Wrapper/Quota.php @@ -26,6 +26,7 @@ namespace OC\Files\Storage\Wrapper; use OCP\Files\Cache\ICacheEntry; +use OC\Files\Filesystem; class Quota extends Wrapper { @@ -39,6 +40,9 @@ class Quota extends Wrapper { */ protected $sizeRoot; + /** @var string $mountPoint */ + protected $mountPoint; + /** * @param array $parameters */ @@ -46,6 +50,7 @@ public function __construct($parameters) { parent::__construct($parameters); $this->quota = $parameters['quota']; $this->sizeRoot = isset($parameters['root']) ? $parameters['root'] : ''; + $this->mountPoint = $parameters['mountPoint'] ?? ''; } /** @@ -142,13 +147,23 @@ public function copy($source, $target) { public function fopen($path, $mode) { $source = $this->storage->fopen($path, $mode); - // if it's a .part file, check if we're trying to overwrite a file $used = \OCP\Files\FileInfo::SPACE_NOT_COMPUTED; + $free = $this->free_space(''); + // if it's a .part file, check if we're trying to overwrite a file if ($this->isPartFile($path)) { $used = $this->getSize($this->stripPartialFileExtension($path)); + + $view = new \OC\Files\View(); + $fullPath = Filesystem::normalizePath("{$this->mountPoint}/{$path}", true, true, true); + $fullPath = $this->stripPartialFileExtension($fullPath); + + $fInfo = $view->getFileInfo($fullPath); + if ($fInfo && $fInfo->isShared()) { + $free = $view->free_space($fullPath); + $used = $fInfo->getSize(); + } } - $free = $this->free_space(''); if ($used >= 0) { // if we're overwriting a file, add the size of that file to the available space // so it's possible to overwrite in case the quota is limited diff --git a/lib/private/legacy/util.php b/lib/private/legacy/util.php index 5508e9be31b9..f2bab14b2ab1 100644 --- a/lib/private/legacy/util.php +++ b/lib/private/legacy/util.php @@ -227,7 +227,12 @@ public static function setupFS($user = '') { $user = $storage->getUser()->getUID(); $quota = OC_Util::getUserQuota($user); if ($quota !== \OCP\Files\FileInfo::SPACE_UNLIMITED) { - return new \OC\Files\Storage\Wrapper\Quota(['storage' => $storage, 'quota' => $quota, 'root' => 'files']); + return new \OC\Files\Storage\Wrapper\Quota([ + 'mountPoint' => $mountPoint, + 'storage' => $storage, + 'quota' => $quota, + 'root' => 'files' + ]); } } } From 1d7ec78f3928270002fb5247ef0e701ff9e5d9de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Pablo=20Villaf=C3=A1=C3=B1ez?= Date: Fri, 18 Mar 2022 15:20:06 +0100 Subject: [PATCH 3/4] Move stripPartialFileExtension method to OC_Util --- lib/private/Encryption/File.php | 3 ++- lib/private/Encryption/Keys/Storage.php | 3 ++- lib/private/Encryption/Util.php | 25 ----------------- .../Files/Storage/Wrapper/Encryption.php | 7 ++--- lib/private/Files/Storage/Wrapper/Quota.php | 26 +++--------------- lib/private/Files/Stream/Checksum.php | 27 ++----------------- lib/private/legacy/util.php | 26 ++++++++++++++++++ tests/lib/Encryption/Keys/StorageTest.php | 20 -------------- tests/lib/Encryption/UtilTest.php | 22 --------------- .../Files/Storage/Wrapper/EncryptionTest.php | 2 -- 10 files changed, 40 insertions(+), 121 deletions(-) diff --git a/lib/private/Encryption/File.php b/lib/private/Encryption/File.php index f70d3907393b..4c9309ad5b5e 100644 --- a/lib/private/Encryption/File.php +++ b/lib/private/Encryption/File.php @@ -24,6 +24,7 @@ namespace OC\Encryption; use OC\Cache\CappedMemoryCache; +use OC_Util; class File implements \OCP\Encryption\IFile { @@ -61,7 +62,7 @@ public function getAccessList($path) { } $ownerPath = \substr($ownerPath, \strlen('/files')); - $ownerPath = $this->util->stripPartialFileExtension($ownerPath); + $ownerPath = OC_Util::stripPartialFileExtension($ownerPath); // first get the shares for the parent and cache the result so that we don't // need to check all parents for every file diff --git a/lib/private/Encryption/Keys/Storage.php b/lib/private/Encryption/Keys/Storage.php index 264dd2d6cb18..8013baf45796 100644 --- a/lib/private/Encryption/Keys/Storage.php +++ b/lib/private/Encryption/Keys/Storage.php @@ -30,6 +30,7 @@ use OCP\Encryption\Keys\IStorage; use OCP\IUserSession; use OC\User\NoUserException; +use OC_Util; class Storage implements IStorage { @@ -89,7 +90,7 @@ public function getUserKey($uid, $keyId, $encryptionModuleId) { * @inheritdoc */ public function getFileKey($path, $keyId, $encryptionModuleId) { - $realFile = $this->util->stripPartialFileExtension($path); + $realFile = OC_Util::stripPartialFileExtension($path); $keyDir = $this->getFileKeyDir($encryptionModuleId, $realFile); $key = $this->getKey($keyDir . $keyId); diff --git a/lib/private/Encryption/Util.php b/lib/private/Encryption/Util.php index 59c0f9a11d47..7e8922b43ff0 100644 --- a/lib/private/Encryption/Util.php +++ b/lib/private/Encryption/Util.php @@ -271,31 +271,6 @@ public function getUidAndFilename($path) { return [$ownerUid, $originalPath]; } - /** - * Remove .path extension from a file path - * @param string $path Path that may identify a .part file - * @return string File path without .part extension - * @note this is needed for reusing keys - */ - public function stripPartialFileExtension($path) { - $extension = \pathinfo($path, PATHINFO_EXTENSION); - - if ($extension === 'part') { - $newLength = \strlen($path) - 5; // 5 = strlen(".part") - $fPath = \substr($path, 0, $newLength); - - // if path also contains a transaction id, we remove it too - $extension = \pathinfo($fPath, PATHINFO_EXTENSION); - if (\substr($extension, 0, 12) === 'ocTransferId') { // 12 = strlen("ocTransferId") - $newLength = \strlen($fPath) - \strlen($extension) -1; - $fPath = \substr($fPath, 0, $newLength); - } - return $fPath; - } else { - return $path; - } - } - public function getUserWithAccessToMountPoint($users, $groups) { $result = []; if (\in_array('all', $users)) { diff --git a/lib/private/Files/Storage/Wrapper/Encryption.php b/lib/private/Files/Storage/Wrapper/Encryption.php index 02381297bbce..8fb02ac5ef3b 100644 --- a/lib/private/Files/Storage/Wrapper/Encryption.php +++ b/lib/private/Files/Storage/Wrapper/Encryption.php @@ -42,6 +42,7 @@ use OCP\Files\Storage; use OCP\ILogger; use OCP\Files\Cache\ICacheEntry; +use OC_Util; class Encryption extends Wrapper { use LocalTempFileTrait; @@ -384,7 +385,7 @@ public function fopen($path, $mode) { if ($this->util->isExcluded($fullPath) === false) { $size = $unencryptedSize = 0; - $realFile = $this->util->stripPartialFileExtension($path); + $realFile = OC_Util::stripPartialFileExtension($path); $targetExists = $this->file_exists($realFile) || $this->file_exists($path); $targetIsEncrypted = false; if ($targetExists) { @@ -942,7 +943,7 @@ protected function readFirstBlock($path) { protected function getHeaderSize($path) { $headerSize = 0; if (!\is_resource($path)) { - $realFile = $this->util->stripPartialFileExtension($path); + $realFile = OC_Util::stripPartialFileExtension($path); if ($this->storage->file_exists($realFile)) { $path = $realFile; } @@ -994,7 +995,7 @@ protected function getHeader($path) { if (\is_resource($path)) { $exists = false; } else { - $realFile = $this->util->stripPartialFileExtension($path); + $realFile = OC_Util::stripPartialFileExtension($path); $exists = $this->storage->file_exists($realFile); if ($exists) { $path = $realFile; diff --git a/lib/private/Files/Storage/Wrapper/Quota.php b/lib/private/Files/Storage/Wrapper/Quota.php index ed59bfd3d2cd..d82c1bd43991 100644 --- a/lib/private/Files/Storage/Wrapper/Quota.php +++ b/lib/private/Files/Storage/Wrapper/Quota.php @@ -27,6 +27,7 @@ use OCP\Files\Cache\ICacheEntry; use OC\Files\Filesystem; +use OC_Util; class Quota extends Wrapper { @@ -151,11 +152,11 @@ public function fopen($path, $mode) { $free = $this->free_space(''); // if it's a .part file, check if we're trying to overwrite a file if ($this->isPartFile($path)) { - $used = $this->getSize($this->stripPartialFileExtension($path)); + $strippedPath = OC_Util::stripPartialFileExtension($path); + $used = $this->getSize($strippedPath); $view = new \OC\Files\View(); - $fullPath = Filesystem::normalizePath("{$this->mountPoint}/{$path}", true, true, true); - $fullPath = $this->stripPartialFileExtension($fullPath); + $fullPath = Filesystem::normalizePath("{$this->mountPoint}/{$strippedPath}", true, true, true); $fInfo = $view->getFileInfo($fullPath); if ($fInfo && $fInfo->isShared()) { @@ -191,25 +192,6 @@ private function isPartFile($path) { return ($extension === 'part'); } - private function stripPartialFileExtension($path) { - $extension = \pathinfo($path, PATHINFO_EXTENSION); - - if ($extension === 'part') { - $newLength = \strlen($path) - 5; // 5 = strlen(".part") - $fPath = \substr($path, 0, $newLength); - - // if path also contains a transaction id, we remove it too - $extension = \pathinfo($fPath, PATHINFO_EXTENSION); - if (\substr($extension, 0, 12) === 'ocTransferId') { // 12 = strlen("ocTransferId") - $newLength = \strlen($fPath) - \strlen($extension) -1; - $fPath = \substr($fPath, 0, $newLength); - } - return $fPath; - } else { - return $path; - } - } - /** * @param \OCP\Files\Storage $sourceStorage * @param string $sourceInternalPath diff --git a/lib/private/Files/Stream/Checksum.php b/lib/private/Files/Stream/Checksum.php index 1b0af03cce92..7982022ad063 100644 --- a/lib/private/Files/Stream/Checksum.php +++ b/lib/private/Files/Stream/Checksum.php @@ -24,6 +24,7 @@ use Icewind\Streams\Wrapper; use OC\Cache\CappedMemoryCache; +use OC_Util; /** * Computes the checksum of the wrapped stream. The checksum can be retrieved with @@ -130,30 +131,6 @@ private function updateHashingContexts($data) { } } - /** - * Remove .part extension from a file path - * @param string $path Path that may identify a .part file - * @return string File path without .part extension - */ - private function stripPartialFileExtension($path) { - $extension = \pathinfo($path, PATHINFO_EXTENSION); - - if ($extension === 'part') { - $newLength = \strlen($path) - 5; // 5 = strlen(".part") - $fPath = \substr($path, 0, $newLength); - - // if path also contains a transaction id, we remove it too - $extension = \pathinfo($fPath, PATHINFO_EXTENSION); - if (\substr($extension, 0, 12) === 'ocTransferId') { // 12 = strlen("ocTransferId") - $newLength = \strlen($fPath) - \strlen($extension) -1; - $fPath = \substr($fPath, 0, $newLength); - } - return $fPath; - } else { - return $path; - } - } - /** * Make checksums available for part files and the original file for which part file has been created * @return bool @@ -166,7 +143,7 @@ public function stream_close() { // If current path belongs to part file, save checksum for original file // As a result, call to getChecksums for original file (of this part file) will // fetch checksum from cache - $originalFilePath = $this->stripPartialFileExtension($currentPath); + $originalFilePath = OC_Util::stripPartialFileExtension($currentPath); if ($originalFilePath !== $currentPath) { self::$checksums[$originalFilePath] = $checksum; } diff --git a/lib/private/legacy/util.php b/lib/private/legacy/util.php index f2bab14b2ab1..4372f0332db6 100644 --- a/lib/private/legacy/util.php +++ b/lib/private/legacy/util.php @@ -1459,6 +1459,32 @@ public static function basename($file) { return \array_pop($t); } + /** + * Remove the ".part" extension from the path. The "ocTransferId" part added for the uploads + * can also be removed. + * @params string $path the path to be cleaned up + * @params bool $stripTransferId whether the method should also remove the "ocTransferId" if it exists + * @return string the cleaned path + */ + public static function stripPartialFileExtension(string $path, bool $stripTransferId = true): string { + $extension = \pathinfo($path, PATHINFO_EXTENSION); + + if ($extension === 'part') { + $newLength = \strlen($path) - 5; // 5 = strlen(".part") + $fPath = \substr($path, 0, $newLength); + + // if path also contains a transaction id, we remove it too + $extension = \pathinfo($fPath, PATHINFO_EXTENSION); + if ($stripTransferId && \substr($extension, 0, 12) === 'ocTransferId') { // 12 = strlen("ocTransferId") + $newLength = \strlen($fPath) - \strlen($extension) -1; + $fPath = \substr($fPath, 0, $newLength); + } + return $fPath; + } else { + return $path; + } + } + /** * A human readable string is generated based on version, channel and build number * diff --git a/tests/lib/Encryption/Keys/StorageTest.php b/tests/lib/Encryption/Keys/StorageTest.php index b6d99cb63054..22b61cd675f0 100644 --- a/tests/lib/Encryption/Keys/StorageTest.php +++ b/tests/lib/Encryption/Keys/StorageTest.php @@ -90,9 +90,6 @@ public function testSetFileKey() { $this->util->expects($this->any()) ->method('getUidAndFilename') ->willReturn(['user1', '/files/foo.txt']); - $this->util->expects($this->any()) - ->method('stripPartialFileExtension') - ->willReturnArgument(0); $this->util->expects($this->any()) ->method('isSystemWideMountPoint') ->willReturn(false); @@ -132,11 +129,6 @@ public function testGetFileKey($path, $strippedPartialName, $originalKeyExists, ['user1/files/foo.txt', ['user1', '/files/foo.txt']], ['user1/files/foo.txt.ocTransferId2111130212.part', ['user1', '/files/foo.txt.ocTransferId2111130212.part']], ]); - // we need to strip away the part file extension in order to reuse a - // existing key if it exists, otherwise versions will break - $this->util->expects($this->once()) - ->method('stripPartialFileExtension') - ->willReturn('user1' . $strippedPartialName); $this->util->expects($this->any()) ->method('isSystemWideMountPoint') ->willReturn(false); @@ -183,9 +175,6 @@ public function testSetFileKeySystemWide() { $this->util->expects($this->any()) ->method('isSystemWideMountPoint') ->willReturn(true); - $this->util->expects($this->any()) - ->method('stripPartialFileExtension') - ->willReturnArgument(0); $this->view->expects($this->once()) ->method('file_put_contents') ->with( @@ -203,9 +192,6 @@ public function testGetFileKeySystemWide() { $this->util->expects($this->any()) ->method('getUidAndFilename') ->willReturn(['user1', '/files/foo.txt']); - $this->util->expects($this->any()) - ->method('stripPartialFileExtension') - ->willReturnArgument(0); $this->util->expects($this->any()) ->method('isSystemWideMountPoint') ->willReturn(true); @@ -383,9 +369,6 @@ public function testDeleteFileKeySystemWide() { $this->util->expects($this->any()) ->method('getUidAndFilename') ->willReturn(['user1', '/files/foo.txt']); - $this->util->expects($this->any()) - ->method('stripPartialFileExtension') - ->willReturnArgument(0); $this->util->expects($this->any()) ->method('isSystemWideMountPoint') ->willReturn(true); @@ -407,9 +390,6 @@ public function testDeleteFileKey() { $this->util->expects($this->any()) ->method('getUidAndFilename') ->willReturn(['user1', '/files/foo.txt']); - $this->util->expects($this->any()) - ->method('stripPartialFileExtension') - ->willReturnArgument(0); $this->util->expects($this->any()) ->method('isSystemWideMountPoint') ->willReturn(false); diff --git a/tests/lib/Encryption/UtilTest.php b/tests/lib/Encryption/UtilTest.php index 40a59d002f62..c4e7dc2cd97e 100644 --- a/tests/lib/Encryption/UtilTest.php +++ b/tests/lib/Encryption/UtilTest.php @@ -168,26 +168,4 @@ public function dataTestIsFile() { ['/user/test.txt', false], ]; } - - /** - * @dataProvider dataTestStripPartialFileExtension - * - * @param string $path - * @param string $expected - */ - public function testStripPartialFileExtension($path, $expected) { - $this->assertSame( - $expected, - $this->util->stripPartialFileExtension($path) - ); - } - - public function dataTestStripPartialFileExtension() { - return [ - ['/foo/test.txt', '/foo/test.txt'], - ['/foo/test.txt.part', '/foo/test.txt'], - ['/foo/test.txt.ocTransferId7567846853.part', '/foo/test.txt'], - ['/foo/test.txt.ocTransferId7567.part', '/foo/test.txt'], - ]; - } } diff --git a/tests/lib/Files/Storage/Wrapper/EncryptionTest.php b/tests/lib/Files/Storage/Wrapper/EncryptionTest.php index 43ac90a5606e..97e1c4898a15 100644 --- a/tests/lib/Files/Storage/Wrapper/EncryptionTest.php +++ b/tests/lib/Files/Storage/Wrapper/EncryptionTest.php @@ -596,8 +596,6 @@ public function testGetHeader($path, $strippedPathExists, $strippedPath) { ->with($path)->willReturn(''); } - $util->expects($this->once())->method('stripPartialFileExtension') - ->with($path)->willReturn($strippedPath); $sourceStorage->expects($this->once()) ->method('file_exists') ->with($strippedPath) From 4e6e9ac2a6fa39253ee88be26984029dc8887ff3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Pablo=20Villaf=C3=A1=C3=B1ez?= Date: Fri, 18 Mar 2022 15:36:27 +0100 Subject: [PATCH 4/4] Fixes for sonarCloud --- apps/dav/lib/Connector/Sabre/File.php | 32 +++++++++++++------- apps/dav/lib/Connector/Sabre/QuotaPlugin.php | 2 +- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php index 3f4fd329e1fd..e948933cd2f0 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -220,17 +220,7 @@ public function put($data) { } if ($result === false) { - $expected = -1; - if (isset($_SERVER['CONTENT_LENGTH'])) { - $expected = $_SERVER['CONTENT_LENGTH']; - } elseif (\is_resource($data)) { - $stat = \fstat($data); - if ($stat !== false) { - $expected = $stat['size']; - } - } elseif (\is_string($data)) { - $expected = \strlen($data); - } + $expected = $this->getExpectedContentLength($data); throw new Exception('Error while copying file to target location (copied bytes: ' . $count . ', expected filesize: ' . $expected . ' )'); } @@ -328,6 +318,26 @@ public function put($data) { return '"' . $this->info->getEtag() . '"'; } + /** + * Figure out the content length for the data. The data could be a resource or a string. + * The $_SERVER['CONTENT_LENGTH'] variable will be prioritized over anything if it's set + * Return -1 if we can't figure out the content length + */ + private function getExpectedContentLength($data) { + $expected = -1; + if (isset($_SERVER['CONTENT_LENGTH'])) { + $expected = $_SERVER['CONTENT_LENGTH']; + } elseif (\is_resource($data)) { + $stat = \fstat($data); + if ($stat !== false) { + $expected = $stat['size']; + } + } elseif (\is_string($data)) { + $expected = \strlen($data); + } + return $expected; + } + private function getPartFileBasePath($path) { $partFileInStorage = \OC::$server->getConfig()->getSystemValue('part_file_in_storage', true); if ($partFileInStorage) { diff --git a/apps/dav/lib/Connector/Sabre/QuotaPlugin.php b/apps/dav/lib/Connector/Sabre/QuotaPlugin.php index 15c273293b0b..105d9c00edc2 100644 --- a/apps/dav/lib/Connector/Sabre/QuotaPlugin.php +++ b/apps/dav/lib/Connector/Sabre/QuotaPlugin.php @@ -131,7 +131,7 @@ public function handleBeforeMove($source, $destination) { public function handleBeforeCopy(RequestInterface $request) { if ($request->getMethod() !== 'COPY') { - return; + return true; } $requestedPath = $request->getPath();