From 241a17226fec2264ba649c49c918368d0555049f Mon Sep 17 00:00:00 2001 From: Ilja Neumann Date: Tue, 7 Feb 2017 11:06:25 +0100 Subject: [PATCH 01/31] Server side checksum verification #26655 --- apps/dav/lib/Connector/Sabre/File.php | 36 ++++ apps/files/lib/Capabilities.php | 4 + .../Files/Storage/Wrapper/Checksum.php | 88 ++++++++++ lib/private/Files/Stream/Checksum.php | 161 ++++++++++++++++++ lib/private/legacy/util.php | 6 + 5 files changed, 295 insertions(+) create mode 100644 lib/private/Files/Storage/Wrapper/Checksum.php create mode 100644 lib/private/Files/Stream/Checksum.php diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php index e35c5f2b076e..952557253f8c 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -34,6 +34,7 @@ namespace OCA\DAV\Connector\Sabre; use OC\Files\Filesystem; +use OC\Files\Storage\Storage; use OCA\DAV\Connector\Sabre\Exception\EntityTooLarge; use OCA\DAV\Connector\Sabre\Exception\FileLocked; use OCA\DAV\Connector\Sabre\Exception\Forbidden as DAVForbiddenException; @@ -134,6 +135,10 @@ public function put($data) { list($count, $result) = \OC_Helper::streamCopy($data, $target); fclose($target); + if (!self::isChecksumValid($partStorage, $internalPartPath)) { + throw new BadRequest('The computed checksum does not match the one received from the client.'); + } + if ($result === false) { $expected = -1; if (isset($_SERVER['CONTENT_LENGTH'])) { @@ -441,6 +446,10 @@ private function createFileChunked($data) { $chunk_handler->file_assemble($partStorage, $partInternalPath); + if (!self::isChecksumValid($partStorage, $partInternalPath)) { + throw new BadRequest('The computed checksum does not match the one received from the client.'); + } + // here is the final atomic rename $renameOkay = $targetStorage->moveFromStorage($partStorage, $partInternalPath, $targetInternalPath); $fileExists = $targetStorage->file_exists($targetInternalPath); @@ -500,6 +509,33 @@ private function createFileChunked($data) { return null; } + /** + * will return true if checksum was not provided in request + * + * @param Storage $storage + * @param $path + * @return bool + */ + private static function isChecksumValid(Storage $storage, $path) { + $meta = $storage->getMetaData($path); + $request = \OC::$server->getRequest(); + + + if (!isset($request->server['HTTP_OC_CHECKSUM']) || !isset($meta['checksum'])) { + // No comparison possible, skip the check + return true; + } + + $expectedChecksum = trim($request->server['HTTP_OC_CHECKSUM']); + $computedChecksum = $meta['checksum']; + + if ($expectedChecksum !== $computedChecksum) { + return false; + } + + return true; + } + /** * Returns whether a part file is needed for the given storage * or whether the file can be assembled/uploaded directly on the diff --git a/apps/files/lib/Capabilities.php b/apps/files/lib/Capabilities.php index ea4fdc1eb318..9a2d90039230 100644 --- a/apps/files/lib/Capabilities.php +++ b/apps/files/lib/Capabilities.php @@ -52,6 +52,10 @@ public function __construct(IConfig $config) { */ public function getCapabilities() { return [ + 'checksums' => [ + 'supportedTypes' => ['SHA1'], + 'preferredUploadType' => 'SHA1' + ], 'files' => [ 'bigfilechunking' => true, 'blacklisted_files' => $this->config->getSystemValue('blacklisted_files', ['.htaccess']), diff --git a/lib/private/Files/Storage/Wrapper/Checksum.php b/lib/private/Files/Storage/Wrapper/Checksum.php new file mode 100644 index 000000000000..780ecc435d45 --- /dev/null +++ b/lib/private/Files/Storage/Wrapper/Checksum.php @@ -0,0 +1,88 @@ + + * + * @copyright Copyright (c) 2017, ownCloud GmbH. + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * + */ +namespace OC\Files\Storage\Wrapper; + +use OC\Files\Stream\Checksum as ChecksumStream; + +/** + * Class Checksum + * + * Computes checksums (default: SHA1) on all files under the /files path. + * The resulting checksum can be retrieved by call getMetadata($path) + * + * @package OC\Files\Storage\Wrapper + */ +class Checksum extends Wrapper { + + /** + * @param array $parameters + */ + public function __construct($parameters) { + if (isset($parameters['algo'])) { + ChecksumStream::setAlgo($parameters['algo']); + } + parent::__construct($parameters); + } + + /** + * @param string $path + * @param string $mode + * @return false|resource + */ + public function fopen($path, $mode) { + $stream = $this->getWrapperStorage()->fopen($path, $mode); + if (!self::requiresChecksum($path)) { + return $stream; + } + + return \OC\Files\Stream\Checksum::wrap($stream, $path); + } + + + /** + * Checksum is only required for everything under files/ + * @param $path + * @return bool + */ + private static function requiresChecksum($path) { + return substr($path, 0, 6) === 'files/'; + } + + /** + * @param string $path + * @param string $data + * @return bool + */ + public function file_put_contents($path, $data) { + return parent::file_put_contents($path, $data); + } + + /** + * @param string $path + * @return array + */ + public function getMetaData($path) { + $parentMetaData = parent::getMetaData($path); + $parentMetaData['checksum'] = ChecksumStream::getChecksum($path); + + return $parentMetaData; + } +} \ No newline at end of file diff --git a/lib/private/Files/Stream/Checksum.php b/lib/private/Files/Stream/Checksum.php new file mode 100644 index 000000000000..b3442d940244 --- /dev/null +++ b/lib/private/Files/Stream/Checksum.php @@ -0,0 +1,161 @@ + + * + * @copyright Copyright (c) 2017, ownCloud GmbH. + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * + */ + +namespace OC\Files\Stream; + + +use Icewind\Streams\Wrapper; + +class Checksum extends Wrapper { + + private static $algo = 'sha1'; + + /** @var resource */ + private $hashCtx; + + /** @var array Key is path, value is the checksum */ + private static $checksums = []; + + public function __construct() { + $this->hashCtx = hash_init(self::$algo); + } + + + /** + * @param $source + * @param $path + * @return resource + */ + public static function wrap($source, $path) { + $context = stream_context_create([ + 'occhecksum' => [ + 'source' => $source, + 'path' => $path + ] + ]); + + return Wrapper::wrapSource( + $source, $context, 'occhecksum', self::class + ); + } + + + /** + * @param string $path + * @param array $options + * @return bool + */ + public function dir_opendir($path, $options) { + return true; + } + + /** + * @param string $path + * @param string $mode + * @param int $options + * @param string $opened_path + * @return bool + */ + public function stream_open($path, $mode, $options, &$opened_path) { + $context = parent::loadContext('occhecksum'); + $this->setSourceStream($context['source']); + + return true; + } + + /** + * @param int $count + * @return string + */ + public function stream_read($count) { + $data = parent::stream_read($count); + hash_update($this->hashCtx, $data); + + return $data; + } + + /** + * @param string $data + * @return int + */ + public function stream_write($data) { + hash_update($this->hashCtx, $data); + + return parent::stream_write($data); + } + + /** + * @return bool + */ + public function stream_close() { + self::$checksums[$this->getPathFromContext()] = sprintf( + '%s:%s', strtoupper(self::$algo), hash_final($this->hashCtx) + ); + + return parent::stream_close(); + } + + /** + * @return mixed + * @return string + */ + private function getPathFromContext() { + $ctx = stream_context_get_options($this->context); + + return $ctx['occhecksum']['path']; + } + + /** + * @param $path + * @return string + */ + public static function getChecksum($path) { + if (!isset(self::$checksums[$path])) { + return null; + } + + return self::$checksums[$path]; + } + + /** + * @return array + */ + public static function getChecksums() { + return self::$checksums; + } + + /** + * @return string + */ + public static function getAlgo() { + return self::$algo; + } + + /** + * @param string $algo + */ + public static function setAlgo($algo) { + self::$algo = $algo; + } + + +} \ No newline at end of file diff --git a/lib/private/legacy/util.php b/lib/private/legacy/util.php index a0019b09f0f7..49e3ca6ac6bb 100644 --- a/lib/private/legacy/util.php +++ b/lib/private/legacy/util.php @@ -180,6 +180,12 @@ public static function setupFS($user = '') { return $storage; }); + // install storage checksum wrapper + \OC\Files\Filesystem::addStorageWrapper('oc_checksum', function ($mountPoint, $storage) { + return new \OC\Files\Storage\Wrapper\Checksum(['storage' => $storage]); + }); + + \OC\Files\Filesystem::addStorageWrapper('oc_encoding', function ($mountPoint, \OCP\Files\Storage $storage, \OCP\Files\Mount\IMountPoint $mount) { if ($mount->getOption('encoding_compatibility', false) && !$storage->instanceOfStorage('\OCA\Files_Sharing\SharedStorage') && !$storage->isLocal()) { return new \OC\Files\Storage\Wrapper\Encoding(['storage' => $storage]); From 8b33c6e8c04de5c88fbd98cfdf6b49413518145b Mon Sep 17 00:00:00 2001 From: Ilja Neumann Date: Tue, 7 Feb 2017 14:57:40 +0100 Subject: [PATCH 02/31] adding file_put_contents implementation to checksum wrapper #26655 --- lib/private/Files/Storage/Wrapper/Checksum.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/private/Files/Storage/Wrapper/Checksum.php b/lib/private/Files/Storage/Wrapper/Checksum.php index 780ecc435d45..66ccb637bcf0 100644 --- a/lib/private/Files/Storage/Wrapper/Checksum.php +++ b/lib/private/Files/Storage/Wrapper/Checksum.php @@ -72,6 +72,10 @@ private static function requiresChecksum($path) { * @return bool */ public function file_put_contents($path, $data) { + $stream = fopen('occhecksum://','r+'); + fwrite($stream, $data); + fclose($stream); + return parent::file_put_contents($path, $data); } From e207e046a58ab94fa21ea8e8c94196bd71b297dc Mon Sep 17 00:00:00 2001 From: Ilja Neumann Date: Tue, 7 Feb 2017 15:11:05 +0100 Subject: [PATCH 03/31] fixing bug where stream was not defined #26655 --- lib/private/Files/Storage/Wrapper/Checksum.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/Files/Storage/Wrapper/Checksum.php b/lib/private/Files/Storage/Wrapper/Checksum.php index 66ccb637bcf0..d900473041c0 100644 --- a/lib/private/Files/Storage/Wrapper/Checksum.php +++ b/lib/private/Files/Storage/Wrapper/Checksum.php @@ -72,7 +72,7 @@ private static function requiresChecksum($path) { * @return bool */ public function file_put_contents($path, $data) { - $stream = fopen('occhecksum://','r+'); + $stream = $this->fopen('php://memory','r+'); fwrite($stream, $data); fclose($stream); From ad8c417353506eed9859b6925d864b834cba1896 Mon Sep 17 00:00:00 2001 From: Ilja Neumann Date: Tue, 7 Feb 2017 15:42:20 +0100 Subject: [PATCH 04/31] fixing bug where wrapper could not write to memory stream#26655 --- lib/private/Files/Storage/Wrapper/Checksum.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/private/Files/Storage/Wrapper/Checksum.php b/lib/private/Files/Storage/Wrapper/Checksum.php index d900473041c0..deda95ba3897 100644 --- a/lib/private/Files/Storage/Wrapper/Checksum.php +++ b/lib/private/Files/Storage/Wrapper/Checksum.php @@ -72,9 +72,11 @@ private static function requiresChecksum($path) { * @return bool */ public function file_put_contents($path, $data) { - $stream = $this->fopen('php://memory','r+'); - fwrite($stream, $data); - fclose($stream); + $memoryStream = fopen('php://memory', 'r+'); + $checksumStream = \OC\Files\Stream\Checksum::wrap($memoryStream, $path); + + fwrite($checksumStream, $data); + fclose($checksumStream); return parent::file_put_contents($path, $data); } From 8af929cb403b557151ff3ba5b4838f19f681e967 Mon Sep 17 00:00:00 2001 From: Ilja Neumann Date: Tue, 7 Feb 2017 17:55:44 +0100 Subject: [PATCH 05/31] fixing priority #26655 --- lib/private/legacy/util.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/legacy/util.php b/lib/private/legacy/util.php index 49e3ca6ac6bb..2dc1d50a8ba5 100644 --- a/lib/private/legacy/util.php +++ b/lib/private/legacy/util.php @@ -183,7 +183,7 @@ public static function setupFS($user = '') { // install storage checksum wrapper \OC\Files\Filesystem::addStorageWrapper('oc_checksum', function ($mountPoint, $storage) { return new \OC\Files\Storage\Wrapper\Checksum(['storage' => $storage]); - }); + }, 1); \OC\Files\Filesystem::addStorageWrapper('oc_encoding', function ($mountPoint, \OCP\Files\Storage $storage, \OCP\Files\Mount\IMountPoint $mount) { From 862506b3a7bbd861bc35c6c9f3ac7cc317265038 Mon Sep 17 00:00:00 2001 From: Ilja Neumann Date: Tue, 7 Feb 2017 18:06:27 +0100 Subject: [PATCH 06/31] fixing broken indents --- lib/private/Files/Stream/Checksum.php | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/private/Files/Stream/Checksum.php b/lib/private/Files/Stream/Checksum.php index b3442d940244..014b2c0ad1ff 100644 --- a/lib/private/Files/Stream/Checksum.php +++ b/lib/private/Files/Stream/Checksum.php @@ -77,7 +77,7 @@ public function dir_opendir($path, $options) { */ public function stream_open($path, $mode, $options, &$opened_path) { $context = parent::loadContext('occhecksum'); - $this->setSourceStream($context['source']); + $this->setSourceStream($context['source']); return true; } @@ -107,7 +107,7 @@ public function stream_write($data) { * @return bool */ public function stream_close() { - self::$checksums[$this->getPathFromContext()] = sprintf( + self::$checksums[$this->getPathFromContext()] = sprintf( '%s:%s', strtoupper(self::$algo), hash_final($this->hashCtx) ); @@ -156,6 +156,4 @@ public static function getAlgo() { public static function setAlgo($algo) { self::$algo = $algo; } - - -} \ No newline at end of file +} From 5d9c4b1c7782a593a93967a2f612853f5e77b66d Mon Sep 17 00:00:00 2001 From: Ilja Neumann Date: Tue, 7 Feb 2017 18:09:37 +0100 Subject: [PATCH 07/31] adding class description #26655 --- lib/private/Files/Stream/Checksum.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/private/Files/Stream/Checksum.php b/lib/private/Files/Stream/Checksum.php index 014b2c0ad1ff..cd4bf4659c8f 100644 --- a/lib/private/Files/Stream/Checksum.php +++ b/lib/private/Files/Stream/Checksum.php @@ -25,6 +25,13 @@ use Icewind\Streams\Wrapper; +/** + * Computes the checksum of the wrapped stream. The checksum can be retrieved with + * getChecksum after the stream is closed. + * + * + * @package OC\Files\Stream + */ class Checksum extends Wrapper { private static $algo = 'sha1'; From 2cf5d164629229921c7d3e72c63a92ce93ba6aef Mon Sep 17 00:00:00 2001 From: Ilja Neumann Date: Wed, 8 Feb 2017 14:53:16 +0100 Subject: [PATCH 08/31] Integration tests for checksuming #26655 --- .../integration/features/bootstrap/WebDav.php | 40 +++++++++++++++++++ .../features/webdav-related.feature | 12 ++++++ 2 files changed, 52 insertions(+) diff --git a/tests/integration/features/bootstrap/WebDav.php b/tests/integration/features/bootstrap/WebDav.php index 071db056d908..946a57e6f499 100644 --- a/tests/integration/features/bootstrap/WebDav.php +++ b/tests/integration/features/bootstrap/WebDav.php @@ -485,6 +485,46 @@ public function userUploadsAFileWithContentTo($user, $content, $destination) } } + + /** + * @When user :user uploads file with checksum :checksum and content :content to :destination + * @param $user + * @param $checksum + * @param $content + * @param $destination + */ + public function userUploadsAFileWithChecksumAndContentTo($user, $checksum, $content, $destination) + { + $file = \GuzzleHttp\Stream\Stream::factory($content); + try { + $this->response = $this->makeDavRequest( + $user, + "PUT", + $destination, + ['OC-Checksum' => $checksum], + $file + ); + } catch (\GuzzleHttp\Exception\BadResponseException $e) { + // 4xx and 5xx responses cause an exception + $this->response = $e->getResponse(); + } + } + + + /** + * @Given file :file does not exist for user :user + * @param string $file + * @param $user + */ + public function fileDoesNotExist($file, $user) { + try { + $this->response = $this->makeDavRequest($user, 'DELETE', $file, []); + } catch (\GuzzleHttp\Exception\BadResponseException $e) { + // 4xx and 5xx responses cause an exception + $this->response = $e->getResponse(); + } + } + /** * @When /^User "([^"]*)" deletes (file|folder) "([^"]*)"$/ * @param string $user diff --git a/tests/integration/features/webdav-related.feature b/tests/integration/features/webdav-related.feature index fb764469592b..e5dd0292eec2 100644 --- a/tests/integration/features/webdav-related.feature +++ b/tests/integration/features/webdav-related.feature @@ -407,6 +407,18 @@ Feature: webdav-related And Downloading file "/myChunkedFile.txt" Then Downloaded content should be "AAAAABBBBBCCCCC" + Scenario: Upload a file where checksum does not match + Given user "user0" exists + And file "/chksumtst.txt" does not exist for user "user0" + And user "user0" uploads file with checksum "SHA1:f005ba11" and content "Some Text" to "/chksumtst.txt" + Then the HTTP status code should be "400" + + Scenario: Upload a file where checksum does match + Given user "user0" exists + And file "/chksumtst.txt" does not exist for user "user0" + And user "user0" uploads file with checksum "SHA1:ce5582148c6f0c1282335b87df5ed4be4b781399" and content "Some Text" to "/chksumtst.txt" + Then the HTTP status code should be "201" + Scenario: A disabled user cannot use webdav Given user "userToBeDisabled" exists And As an "admin" From ee1c2014a693bf9f5ae1298210c9dfa2f0a577ec Mon Sep 17 00:00:00 2001 From: Ilja Neumann Date: Wed, 8 Feb 2017 15:06:48 +0100 Subject: [PATCH 09/31] - adding memory capped cache for paths - do not register checksum wrapper for #26655 --- lib/private/Files/Stream/Checksum.php | 16 +++++++++++++--- lib/private/legacy/util.php | 9 +++++++-- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/lib/private/Files/Stream/Checksum.php b/lib/private/Files/Stream/Checksum.php index cd4bf4659c8f..d1ded83008de 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; /** * Computes the checksum of the wrapped stream. The checksum can be retrieved with @@ -39,11 +40,16 @@ class Checksum extends Wrapper { /** @var resource */ private $hashCtx; - /** @var array Key is path, value is the checksum */ - private static $checksums = []; + /** @var CappedMemoryCache Key is path, value is the checksum */ + private static $checksums; + public function __construct() { $this->hashCtx = hash_init(self::$algo); + + if (!self::$checksums) { + self::$checksums = new CappedMemoryCache(); + } } @@ -144,9 +150,13 @@ public static function getChecksum($path) { } /** - * @return array + * @return CappedMemoryCache */ public static function getChecksums() { + if (!self::$checksums) { + self::$checksums = new CappedMemoryCache(); + } + return self::$checksums; } diff --git a/lib/private/legacy/util.php b/lib/private/legacy/util.php index 2dc1d50a8ba5..19085c78b155 100644 --- a/lib/private/legacy/util.php +++ b/lib/private/legacy/util.php @@ -181,8 +181,13 @@ public static function setupFS($user = '') { }); // install storage checksum wrapper - \OC\Files\Filesystem::addStorageWrapper('oc_checksum', function ($mountPoint, $storage) { - return new \OC\Files\Storage\Wrapper\Checksum(['storage' => $storage]); + \OC\Files\Filesystem::addStorageWrapper('oc_checksum', function ($mountPoint, \OCP\Files\Storage\IStorage $storage) { + if (!$storage->instanceOfStorage('\OCA\Files_Sharing\SharedStorage')) { + return new \OC\Files\Storage\Wrapper\Checksum(['storage' => $storage]); + } + + return $storage; + }, 1); From fd277fd83e28a070b967e3e756d651416e524066 Mon Sep 17 00:00:00 2001 From: Ilja Neumann Date: Wed, 8 Feb 2017 17:19:20 +0100 Subject: [PATCH 10/31] no mimetype handling code #26655 --- lib/private/Files/Storage/Wrapper/Checksum.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/private/Files/Storage/Wrapper/Checksum.php b/lib/private/Files/Storage/Wrapper/Checksum.php index deda95ba3897..1afa65a85ca2 100644 --- a/lib/private/Files/Storage/Wrapper/Checksum.php +++ b/lib/private/Files/Storage/Wrapper/Checksum.php @@ -89,6 +89,11 @@ public function getMetaData($path) { $parentMetaData = parent::getMetaData($path); $parentMetaData['checksum'] = ChecksumStream::getChecksum($path); + // Need to investigate more + if (!isset($parentMetaData['mimetype'])) { + $parentMetaData['mimetype'] = ''; + } + return $parentMetaData; } } \ No newline at end of file From 395de64dabf507073bc7053a095259efbd20f1b0 Mon Sep 17 00:00:00 2001 From: Ilja Neumann Date: Thu, 9 Feb 2017 15:11:54 +0100 Subject: [PATCH 11/31] Fixing failing testExpireOldFilesShared by puting it before testExpireOldFiles. If testExpireOldFiles runs before testExpireOldFilesShared, the second test fails randomly. #26655 --- apps/files_trashbin/tests/TrashbinTest.php | 88 +++++++++++----------- 1 file changed, 46 insertions(+), 42 deletions(-) diff --git a/apps/files_trashbin/tests/TrashbinTest.php b/apps/files_trashbin/tests/TrashbinTest.php index c92fa4a8f520..c1d728649268 100644 --- a/apps/files_trashbin/tests/TrashbinTest.php +++ b/apps/files_trashbin/tests/TrashbinTest.php @@ -146,50 +146,8 @@ protected function tearDown() { parent::tearDown(); } - /** - * test expiration of files older then the max storage time defined for the trash - */ - public function testExpireOldFiles() { - - $currentTime = time(); - $expireAt = $currentTime - 2 * 24 * 60 * 60; - $expiredDate = $currentTime - 3 * 24 * 60 * 60; - - // create some files - \OC\Files\Filesystem::file_put_contents('file1.txt', 'file1'); - \OC\Files\Filesystem::file_put_contents('file2.txt', 'file2'); - \OC\Files\Filesystem::file_put_contents('file3.txt', 'file3'); - - // delete them so that they end up in the trash bin - \OC\Files\Filesystem::unlink('file1.txt'); - \OC\Files\Filesystem::unlink('file2.txt'); - \OC\Files\Filesystem::unlink('file3.txt'); - - //make sure that files are in the trash bin - $filesInTrash = OCA\Files_Trashbin\Helper::getTrashFiles('/', self::TEST_TRASHBIN_USER1, 'name'); - $this->assertSame(3, count($filesInTrash)); - // every second file will get a date in the past so that it will get expired - $manipulatedList = $this->manipulateDeleteTime($filesInTrash, $this->trashRoot1, $expiredDate); - - $testClass = new TrashbinForTesting(); - list($sizeOfDeletedFiles, $count) = $testClass->dummyDeleteExpiredFiles($manipulatedList, $expireAt); - - $this->assertSame(10, $sizeOfDeletedFiles); - $this->assertSame(2, $count); - - // only file2.txt should be left - $remainingFiles = array_slice($manipulatedList, $count); - $this->assertSame(1, count($remainingFiles)); - $remainingFile = reset($remainingFiles); - $this->assertSame('file2.txt', $remainingFile['name']); - // check that file1.txt and file3.txt was really deleted - $newTrashContent = OCA\Files_Trashbin\Helper::getTrashFiles('/', self::TEST_TRASHBIN_USER1); - $this->assertSame(1, count($newTrashContent)); - $element = reset($newTrashContent); - $this->assertSame('file2.txt', $element['name']); - } /** * test expiration of files older then the max storage time defined for the trash @@ -267,6 +225,52 @@ public function testExpireOldFilesShared() { $this->verifyArray($filesInTrashUser1AfterDelete, ['user1-2.txt', 'user1-4.txt']); } + /** + * test expiration of files older then the max storage time defined for the trash + */ + public function testExpireOldFiles() { + + $currentTime = time(); + $expireAt = $currentTime - 2 * 24 * 60 * 60; + $expiredDate = $currentTime - 3 * 24 * 60 * 60; + + // create some files + \OC\Files\Filesystem::file_put_contents('file1.txt', 'file1'); + \OC\Files\Filesystem::file_put_contents('file2.txt', 'file2'); + \OC\Files\Filesystem::file_put_contents('file3.txt', 'file3'); + + // delete them so that they end up in the trash bin + \OC\Files\Filesystem::unlink('file1.txt'); + \OC\Files\Filesystem::unlink('file2.txt'); + \OC\Files\Filesystem::unlink('file3.txt'); + + //make sure that files are in the trash bin + $filesInTrash = OCA\Files_Trashbin\Helper::getTrashFiles('/', self::TEST_TRASHBIN_USER1, 'name'); + $this->assertSame(3, count($filesInTrash)); + + // every second file will get a date in the past so that it will get expired + $manipulatedList = $this->manipulateDeleteTime($filesInTrash, $this->trashRoot1, $expiredDate); + + $testClass = new TrashbinForTesting(); + list($sizeOfDeletedFiles, $count) = $testClass->dummyDeleteExpiredFiles($manipulatedList, $expireAt); + + $this->assertSame(10, $sizeOfDeletedFiles); + $this->assertSame(2, $count); + + // only file2.txt should be left + $remainingFiles = array_slice($manipulatedList, $count); + $this->assertSame(1, count($remainingFiles)); + $remainingFile = reset($remainingFiles); + $this->assertSame('file2.txt', $remainingFile['name']); + + // check that file1.txt and file3.txt was really deleted + $newTrashContent = OCA\Files_Trashbin\Helper::getTrashFiles('/', self::TEST_TRASHBIN_USER1); + $this->assertSame(1, count($newTrashContent)); + $element = reset($newTrashContent); + $this->assertSame('file2.txt', $element['name']); + } + + /** * verify that the array contains the expected results * From 2a6b6265e6e824f8cf57b666b730b88cd15260af Mon Sep 17 00:00:00 2001 From: Ilja Neumann Date: Thu, 9 Feb 2017 15:15:42 +0100 Subject: [PATCH 12/31] Checksum only on write #26655 --- lib/private/Files/Storage/Wrapper/Checksum.php | 11 ++++++----- lib/private/Files/Stream/Checksum.php | 2 +- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/private/Files/Storage/Wrapper/Checksum.php b/lib/private/Files/Storage/Wrapper/Checksum.php index 1afa65a85ca2..66994fe4d101 100644 --- a/lib/private/Files/Storage/Wrapper/Checksum.php +++ b/lib/private/Files/Storage/Wrapper/Checksum.php @@ -49,7 +49,7 @@ public function __construct($parameters) { */ public function fopen($path, $mode) { $stream = $this->getWrapperStorage()->fopen($path, $mode); - if (!self::requiresChecksum($path)) { + if (!self::requiresChecksum($path, $mode)) { return $stream; } @@ -59,11 +59,12 @@ public function fopen($path, $mode) { /** * Checksum is only required for everything under files/ + * @param $mode * @param $path * @return bool */ - private static function requiresChecksum($path) { - return substr($path, 0, 6) === 'files/'; + private static function requiresChecksum($path, $mode) { + return substr($path, 0, 6) === 'files/' && $mode != 'r'; } /** @@ -78,7 +79,7 @@ public function file_put_contents($path, $data) { fwrite($checksumStream, $data); fclose($checksumStream); - return parent::file_put_contents($path, $data); + return $this->getWrapperStorage()->file_put_contents($path, $data); } /** @@ -86,7 +87,7 @@ public function file_put_contents($path, $data) { * @return array */ public function getMetaData($path) { - $parentMetaData = parent::getMetaData($path); + $parentMetaData = $this->getWrapperStorage()->getMetaData($path); $parentMetaData['checksum'] = ChecksumStream::getChecksum($path); // Need to investigate more diff --git a/lib/private/Files/Stream/Checksum.php b/lib/private/Files/Stream/Checksum.php index d1ded83008de..ae622d2fc7a7 100644 --- a/lib/private/Files/Stream/Checksum.php +++ b/lib/private/Files/Stream/Checksum.php @@ -78,7 +78,7 @@ public static function wrap($source, $path) { * @return bool */ public function dir_opendir($path, $options) { - return true; + return false; } /** From d6ba656af4aba70e2edf9acccc19f078da14f5ae Mon Sep 17 00:00:00 2001 From: Ilja Neumann Date: Thu, 9 Feb 2017 15:16:16 +0100 Subject: [PATCH 13/31] strong comparison --- lib/private/Files/Storage/Wrapper/Checksum.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/Files/Storage/Wrapper/Checksum.php b/lib/private/Files/Storage/Wrapper/Checksum.php index 66994fe4d101..0015fcd16ff1 100644 --- a/lib/private/Files/Storage/Wrapper/Checksum.php +++ b/lib/private/Files/Storage/Wrapper/Checksum.php @@ -64,7 +64,7 @@ public function fopen($path, $mode) { * @return bool */ private static function requiresChecksum($path, $mode) { - return substr($path, 0, 6) === 'files/' && $mode != 'r'; + return substr($path, 0, 6) === 'files/' && $mode !== 'r'; } /** From 18c632ef1740de49dea6f15f31c8a0cd080218e8 Mon Sep 17 00:00:00 2001 From: Ilja Neumann Date: Thu, 9 Feb 2017 15:36:53 +0100 Subject: [PATCH 14/31] Reverting accidentally commited testcode #26655 --- lib/private/Files/Stream/Checksum.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/Files/Stream/Checksum.php b/lib/private/Files/Stream/Checksum.php index ae622d2fc7a7..d1ded83008de 100644 --- a/lib/private/Files/Stream/Checksum.php +++ b/lib/private/Files/Stream/Checksum.php @@ -78,7 +78,7 @@ public static function wrap($source, $path) { * @return bool */ public function dir_opendir($path, $options) { - return false; + return true; } /** From fc780dbe886c7e755b21e93e9e56b6220dacb012 Mon Sep 17 00:00:00 2001 From: Ilja Neumann Date: Thu, 9 Feb 2017 17:58:01 +0100 Subject: [PATCH 15/31] Fixing basic lock test fail #26655 --- lib/private/Files/Stream/Checksum.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/private/Files/Stream/Checksum.php b/lib/private/Files/Stream/Checksum.php index d1ded83008de..821fc056fa52 100644 --- a/lib/private/Files/Stream/Checksum.php +++ b/lib/private/Files/Stream/Checksum.php @@ -127,6 +127,13 @@ public function stream_close() { return parent::stream_close(); } + public function dir_closedir() { + if (!isset($this->source)) { + return false; + } + return parent::dir_closedir(); + } + /** * @return mixed * @return string From 5e2ca3c02c28fa434e336e4410c279e08af727ff Mon Sep 17 00:00:00 2001 From: Ilja Neumann Date: Thu, 9 Feb 2017 19:12:16 +0100 Subject: [PATCH 16/31] #26655 Moved MoveFileIntoSharedFolderAsRecepient up because it has some weird sideeffects if other tests are running before it, couldn`t find out the root cause, test runs ok in isolation. --- apps/files_versions/tests/VersioningTest.php | 101 ++++++++++--------- 1 file changed, 51 insertions(+), 50 deletions(-) diff --git a/apps/files_versions/tests/VersioningTest.php b/apps/files_versions/tests/VersioningTest.php index 3dd493693277..7d9cd6e59a68 100644 --- a/apps/files_versions/tests/VersioningTest.php +++ b/apps/files_versions/tests/VersioningTest.php @@ -117,6 +117,57 @@ protected function tearDown() { parent::tearDown(); } + + public function testMoveFileIntoSharedFolderAsRecipient() { + + \OC\Files\Filesystem::mkdir('folder1'); + $fileInfo = \OC\Files\Filesystem::getFileInfo('folder1'); + + $node = \OC::$server->getUserFolder(self::TEST_VERSIONS_USER)->get('folder1'); + $share = \OC::$server->getShareManager()->newShare(); + $share->setNode($node) + ->setShareType(\OCP\Share::SHARE_TYPE_USER) + ->setSharedBy(self::TEST_VERSIONS_USER) + ->setSharedWith(self::TEST_VERSIONS_USER2) + ->setPermissions(\OCP\Constants::PERMISSION_ALL); + $share = \OC::$server->getShareManager()->createShare($share); + + self::loginHelper(self::TEST_VERSIONS_USER2); + $versionsFolder2 = '/' . self::TEST_VERSIONS_USER2 . '/files_versions'; + \OC\Files\Filesystem::file_put_contents('test.txt', 'test file'); + + $t1 = time(); + // second version is two weeks older, this way we make sure that no + // version will be expired + $t2 = $t1 - 60 * 60 * 24 * 14; + + $this->rootView->mkdir($versionsFolder2); + // create some versions + $v1 = $versionsFolder2 . '/test.txt.v' . $t1; + $v2 = $versionsFolder2 . '/test.txt.v' . $t2; + + $this->rootView->file_put_contents($v1, 'version1'); + $this->rootView->file_put_contents($v2, 'version2'); + + // move file into the shared folder as recipient + \OC\Files\Filesystem::rename('/test.txt', '/folder1/test.txt'); + + $this->assertFalse($this->rootView->file_exists($v1)); + $this->assertFalse($this->rootView->file_exists($v2)); + + self::loginHelper(self::TEST_VERSIONS_USER); + + $versionsFolder1 = '/' . self::TEST_VERSIONS_USER . '/files_versions'; + + $v1Renamed = $versionsFolder1 . '/folder1/test.txt.v' . $t1; + $v2Renamed = $versionsFolder1 . '/folder1/test.txt.v' . $t2; + + $this->assertTrue($this->rootView->file_exists($v1Renamed)); + $this->assertTrue($this->rootView->file_exists($v2Renamed)); + + \OC::$server->getShareManager()->deleteShare($share); + } + /** * @medium * test expire logic @@ -378,56 +429,6 @@ public function testMoveFolder() { } - public function testMoveFileIntoSharedFolderAsRecipient() { - - \OC\Files\Filesystem::mkdir('folder1'); - $fileInfo = \OC\Files\Filesystem::getFileInfo('folder1'); - - $node = \OC::$server->getUserFolder(self::TEST_VERSIONS_USER)->get('folder1'); - $share = \OC::$server->getShareManager()->newShare(); - $share->setNode($node) - ->setShareType(\OCP\Share::SHARE_TYPE_USER) - ->setSharedBy(self::TEST_VERSIONS_USER) - ->setSharedWith(self::TEST_VERSIONS_USER2) - ->setPermissions(\OCP\Constants::PERMISSION_ALL); - $share = \OC::$server->getShareManager()->createShare($share); - - self::loginHelper(self::TEST_VERSIONS_USER2); - $versionsFolder2 = '/' . self::TEST_VERSIONS_USER2 . '/files_versions'; - \OC\Files\Filesystem::file_put_contents('test.txt', 'test file'); - - $t1 = time(); - // second version is two weeks older, this way we make sure that no - // version will be expired - $t2 = $t1 - 60 * 60 * 24 * 14; - - $this->rootView->mkdir($versionsFolder2); - // create some versions - $v1 = $versionsFolder2 . '/test.txt.v' . $t1; - $v2 = $versionsFolder2 . '/test.txt.v' . $t2; - - $this->rootView->file_put_contents($v1, 'version1'); - $this->rootView->file_put_contents($v2, 'version2'); - - // move file into the shared folder as recipient - \OC\Files\Filesystem::rename('/test.txt', '/folder1/test.txt'); - - $this->assertFalse($this->rootView->file_exists($v1)); - $this->assertFalse($this->rootView->file_exists($v2)); - - self::loginHelper(self::TEST_VERSIONS_USER); - - $versionsFolder1 = '/' . self::TEST_VERSIONS_USER . '/files_versions'; - - $v1Renamed = $versionsFolder1 . '/folder1/test.txt.v' . $t1; - $v2Renamed = $versionsFolder1 . '/folder1/test.txt.v' . $t2; - - $this->assertTrue($this->rootView->file_exists($v1Renamed)); - $this->assertTrue($this->rootView->file_exists($v2Renamed)); - - \OC::$server->getShareManager()->deleteShare($share); - } - public function testMoveFolderIntoSharedFolderAsRecipient() { \OC\Files\Filesystem::mkdir('folder1'); From 4cb2c05085495dbbcb83c4310bfb0a159653d632 Mon Sep 17 00:00:00 2001 From: Ilja Neumann Date: Thu, 9 Feb 2017 20:09:07 +0100 Subject: [PATCH 17/31] Adding checksum download test #26655 --- tests/integration/features/webdav-related.feature | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/integration/features/webdav-related.feature b/tests/integration/features/webdav-related.feature index e5dd0292eec2..deb998fb06e7 100644 --- a/tests/integration/features/webdav-related.feature +++ b/tests/integration/features/webdav-related.feature @@ -419,6 +419,14 @@ Feature: webdav-related And user "user0" uploads file with checksum "SHA1:ce5582148c6f0c1282335b87df5ed4be4b781399" and content "Some Text" to "/chksumtst.txt" Then the HTTP status code should be "201" + Scenario: Uploaded file should have the same checksum when downloaded + Given user "user0" exists + And file "/chksumtst.txt" does not exist for user "user0" + And user "user0" uploads file with checksum "SHA1:ce5582148c6f0c1282335b87df5ed4be4b781399" and content "Some Text" to "/chksumtst.txt" + When Downloading file "/chksumtst.txt" as "user0" + Then The following headers should be set + | OC-Checksum | SHA1:ce5582148c6f0c1282335b87df5ed4be4b781399 | + Scenario: A disabled user cannot use webdav Given user "userToBeDisabled" exists And As an "admin" From b9852612698b047b29371191831ef73f846bf88a Mon Sep 17 00:00:00 2001 From: Ilja Neumann Date: Fri, 10 Feb 2017 19:59:38 +0100 Subject: [PATCH 18/31] Multiple checksums support + tests#26655 --- apps/dav/lib/Connector/Sabre/File.php | 39 ++++++----- .../Files/Storage/Wrapper/Checksum.php | 17 ++--- lib/private/Files/Stream/Checksum.php | 70 ++++++++++--------- tests/integration/features/checksums.feature | 42 ++++------- .../features/webdav-related.feature | 2 +- 5 files changed, 80 insertions(+), 90 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php index 952557253f8c..3140d127c526 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -223,14 +223,12 @@ public function put($data) { $this->refreshInfo(); - if (isset($request->server['HTTP_OC_CHECKSUM'])) { - $checksum = trim($request->server['HTTP_OC_CHECKSUM']); - $this->fileView->putFileInfo($this->path, ['checksum' => $checksum]); - $this->refreshInfo(); - } else if ($this->getChecksum() !== null && $this->getChecksum() !== '') { - $this->fileView->putFileInfo($this->path, ['checksum' => '']); - $this->refreshInfo(); - } + $this->fileView->putFileInfo( + $this->path, + ['checksum' => $partStorage->getMetaData($internalPartPath)['checksum']] + ); + + $this->refreshInfo(); } catch (StorageNotAvailableException $e) { throw new ServiceUnavailable("Failed to check file size: " . $e->getMessage()); @@ -488,13 +486,20 @@ private function createFileChunked($data) { // FIXME: should call refreshInfo but can't because $this->path is not the of the final file $info = $this->fileView->getFileInfo($targetPath); - if (isset($request->server['HTTP_OC_CHECKSUM'])) { - $checksum = trim($request->server['HTTP_OC_CHECKSUM']); - $this->fileView->putFileInfo($targetPath, ['checksum' => $checksum]); - } else if ($info->getChecksum() !== null && $info->getChecksum() !== '') { - $this->fileView->putFileInfo($this->path, ['checksum' => '']); + + if (isset($partStorage) && isset($partInternalPath)) { + $checksums = $partStorage->getMetaData($partInternalPath)['checksum']; + } else { + $checksums = $targetStorage->getMetaData($targetInternalPath)['checksum']; } + $this->fileView->putFileInfo( + $targetPath, + ['checksum' => $checksums] + ); + + $this->refreshInfo(); + $this->fileView->unlockFile($targetPath, ILockingProvider::LOCK_SHARED); return $info->getEtag(); @@ -520,20 +525,16 @@ private static function isChecksumValid(Storage $storage, $path) { $meta = $storage->getMetaData($path); $request = \OC::$server->getRequest(); - if (!isset($request->server['HTTP_OC_CHECKSUM']) || !isset($meta['checksum'])) { // No comparison possible, skip the check return true; } $expectedChecksum = trim($request->server['HTTP_OC_CHECKSUM']); - $computedChecksum = $meta['checksum']; + $computedChecksums = $meta['checksum']; - if ($expectedChecksum !== $computedChecksum) { - return false; - } + return strpos($computedChecksums, $expectedChecksum) !== false; - return true; } /** diff --git a/lib/private/Files/Storage/Wrapper/Checksum.php b/lib/private/Files/Storage/Wrapper/Checksum.php index 0015fcd16ff1..917df77a84db 100644 --- a/lib/private/Files/Storage/Wrapper/Checksum.php +++ b/lib/private/Files/Storage/Wrapper/Checksum.php @@ -32,15 +32,6 @@ */ class Checksum extends Wrapper { - /** - * @param array $parameters - */ - public function __construct($parameters) { - if (isset($parameters['algo'])) { - ChecksumStream::setAlgo($parameters['algo']); - } - parent::__construct($parameters); - } /** * @param string $path @@ -87,8 +78,14 @@ public function file_put_contents($path, $data) { * @return array */ public function getMetaData($path) { + $checksumString = ''; + + foreach (ChecksumStream::getChecksums($path) as $algo => $checksum) { + $checksumString .= sprintf('%s:%s ', strtoupper($algo), $checksum); + } + $parentMetaData = $this->getWrapperStorage()->getMetaData($path); - $parentMetaData['checksum'] = ChecksumStream::getChecksum($path); + $parentMetaData['checksum'] = rtrim($checksumString); // Need to investigate more if (!isset($parentMetaData['mimetype'])) { diff --git a/lib/private/Files/Stream/Checksum.php b/lib/private/Files/Stream/Checksum.php index 821fc056fa52..ef564ed959d8 100644 --- a/lib/private/Files/Stream/Checksum.php +++ b/lib/private/Files/Stream/Checksum.php @@ -35,17 +35,18 @@ */ class Checksum extends Wrapper { - private static $algo = 'sha1'; + /** @var resource[] */ + private $hashingContexts; - /** @var resource */ - private $hashCtx; - - /** @var CappedMemoryCache Key is path, value is the checksum */ + /** @var CappedMemoryCache Key is path, value is array of checksums */ private static $checksums; - public function __construct() { - $this->hashCtx = hash_init(self::$algo); + public function __construct(array $algos = ['sha1', 'md5', 'adler32']) { + + foreach ($algos as $algo) { + $this->hashingContexts[$algo] = hash_init($algo); + } if (!self::$checksums) { self::$checksums = new CappedMemoryCache(); @@ -101,7 +102,7 @@ public function stream_open($path, $mode, $options, &$opened_path) { */ public function stream_read($count) { $data = parent::stream_read($count); - hash_update($this->hashCtx, $data); + $this->updateHashingContexts($data); return $data; } @@ -111,22 +112,39 @@ public function stream_read($count) { * @return int */ public function stream_write($data) { - hash_update($this->hashCtx, $data); - + $this->updateHashingContexts($data); return parent::stream_write($data); } + private function updateHashingContexts($data) { + foreach ($this->hashingContexts as $ctx) { + hash_update($ctx, $data); + } + } + /** * @return bool */ public function stream_close() { - self::$checksums[$this->getPathFromContext()] = sprintf( - '%s:%s', strtoupper(self::$algo), hash_final($this->hashCtx) - ); + $currentPath = $this->getPathFromStreamContext(); + self::$checksums[$currentPath] = $this->finalizeHashingContexts(); return parent::stream_close(); } + /** + * @return array + */ + private function finalizeHashingContexts() { + $hashes = []; + + foreach ($this->hashingContexts as $algo => $ctx) { + $hashes[$algo] = hash_final($ctx); + } + + return $hashes; + } + public function dir_closedir() { if (!isset($this->source)) { return false; @@ -138,7 +156,7 @@ public function dir_closedir() { * @return mixed * @return string */ - private function getPathFromContext() { + private function getPathFromStreamContext() { $ctx = stream_context_get_options($this->context); return $ctx['occhecksum']['path']; @@ -146,38 +164,26 @@ private function getPathFromContext() { /** * @param $path - * @return string + * @return array */ - public static function getChecksum($path) { + public static function getChecksums($path) { if (!isset(self::$checksums[$path])) { - return null; + return []; } return self::$checksums[$path]; } /** + * For debugging + * * @return CappedMemoryCache */ - public static function getChecksums() { + public static function getChecksumsForAllPaths() { if (!self::$checksums) { self::$checksums = new CappedMemoryCache(); } return self::$checksums; } - - /** - * @return string - */ - public static function getAlgo() { - return self::$algo; - } - - /** - * @param string $algo - */ - public static function setAlgo($algo) { - self::$algo = $algo; - } } diff --git a/tests/integration/features/checksums.feature b/tests/integration/features/checksums.feature index d391e93afe8d..303fd43bba16 100644 --- a/tests/integration/features/checksums.feature +++ b/tests/integration/features/checksums.feature @@ -9,68 +9,54 @@ Feature: checksums Given user "user0" exists And user "user0" uploads file "data/textfile.txt" to "/myChecksumFile.txt" with checksum "MD5:d70b40f177b14b470d1756a3c12b963a" When user "user0" request the checksum of "/myChecksumFile.txt" via propfind - Then The webdav checksum should match "MD5:d70b40f177b14b470d1756a3c12b963a" + Then The webdav checksum should match "SHA1:3ee962b839762adb0ad8ba6023a4690be478de6f MD5:d70b40f177b14b470d1756a3c12b963a ADLER32:8ae90960" Scenario: Uploading a file with checksum should return the checksum in the download header Given user "user0" exists And user "user0" uploads file "data/textfile.txt" to "/myChecksumFile.txt" with checksum "MD5:d70b40f177b14b470d1756a3c12b963a" When user "user0" downloads the file "/myChecksumFile.txt" - Then The header checksum should match "MD5:d70b40f177b14b470d1756a3c12b963a" + Then The header checksum should match "SHA1:3ee962b839762adb0ad8ba6023a4690be478de6f MD5:d70b40f177b14b470d1756a3c12b963a ADLER32:8ae90960" Scenario: Moving a file with checksum should return the checksum in the propfind Given user "user0" exists And user "user0" uploads file "data/textfile.txt" to "/myChecksumFile.txt" with checksum "MD5:d70b40f177b14b470d1756a3c12b963a" When User "user0" moved file "/myChecksumFile.txt" to "/myMovedChecksumFile.txt" And user "user0" request the checksum of "/myMovedChecksumFile.txt" via propfind - Then The webdav checksum should match "MD5:d70b40f177b14b470d1756a3c12b963a" + Then The webdav checksum should match "SHA1:3ee962b839762adb0ad8ba6023a4690be478de6f MD5:d70b40f177b14b470d1756a3c12b963a ADLER32:8ae90960" Scenario: Moving file with checksum should return the checksum in the download header Given user "user0" exists And user "user0" uploads file "data/textfile.txt" to "/myChecksumFile.txt" with checksum "MD5:d70b40f177b14b470d1756a3c12b963a" When User "user0" moved file "/myChecksumFile.txt" to "/myMovedChecksumFile.txt" And user "user0" downloads the file "/myMovedChecksumFile.txt" - Then The header checksum should match "MD5:d70b40f177b14b470d1756a3c12b963a" + Then The header checksum should match "SHA1:3ee962b839762adb0ad8ba6023a4690be478de6f MD5:d70b40f177b14b470d1756a3c12b963a ADLER32:8ae90960" Scenario: Copying a file with checksum should return the checksum in the propfind Given user "user0" exists And user "user0" uploads file "data/textfile.txt" to "/myChecksumFile.txt" with checksum "MD5:d70b40f177b14b470d1756a3c12b963a" When User "user0" copied file "/myChecksumFile.txt" to "/myChecksumFileCopy.txt" And user "user0" request the checksum of "/myChecksumFileCopy.txt" via propfind - Then The webdav checksum should match "MD5:d70b40f177b14b470d1756a3c12b963a" + Then The webdav checksum should match "SHA1:3ee962b839762adb0ad8ba6023a4690be478de6f MD5:d70b40f177b14b470d1756a3c12b963a ADLER32:8ae90960" Scenario: Copying file with checksum should return the checksum in the download header Given user "user0" exists And user "user0" uploads file "data/textfile.txt" to "/myChecksumFile.txt" with checksum "MD5:d70b40f177b14b470d1756a3c12b963a" When User "user0" copied file "/myChecksumFile.txt" to "/myChecksumFileCopy.txt" And user "user0" downloads the file "/myChecksumFileCopy.txt" - Then The header checksum should match "MD5:d70b40f177b14b470d1756a3c12b963a" - - Scenario: Overwriting a file with checksum should remove the checksum and not return it in the propfind - Given user "user0" exists - And user "user0" uploads file "data/textfile.txt" to "/myChecksumFile.txt" with checksum "MD5:d70b40f177b14b470d1756a3c12b963a" - When user "user0" uploads file "data/textfile.txt" to "/myChecksumFile.txt" - And user "user0" request the checksum of "/myChecksumFile.txt" via propfind - Then The webdav checksum should be empty - - Scenario: Overwriting a file with checksum should remove the checksum and not return it in the download header - Given user "user0" exists - And user "user0" uploads file "data/textfile.txt" to "/myChecksumFile.txt" with checksum "MD5:d70b40f177b14b470d1756a3c12b963a" - When user "user0" uploads file "data/textfile.txt" to "/myChecksumFile.txt" - And user "user0" downloads the file "/myChecksumFile.txt" - Then The OC-Checksum header should not be there + Then The header checksum should match "SHA1:3ee962b839762adb0ad8ba6023a4690be478de6f MD5:d70b40f177b14b470d1756a3c12b963a ADLER32:8ae90960" Scenario: Uploading a chunked file with checksum should return the checksum in the propfind Given user "user0" exists - And user "user0" uploads chunk file "1" of "3" with "AAAAA" to "/myChecksumFile.txt" with checksum "MD5:e892fdd61a74bc89cd05673cc2e22f88" - And user "user0" uploads chunk file "2" of "3" with "BBBBB" to "/myChecksumFile.txt" with checksum "MD5:e892fdd61a74bc89cd05673cc2e22f88" - And user "user0" uploads chunk file "3" of "3" with "CCCCC" to "/myChecksumFile.txt" with checksum "MD5:e892fdd61a74bc89cd05673cc2e22f88" + And user "user0" uploads chunk file "1" of "3" with "AAAAA" to "/myChecksumFile.txt" with checksum "MD5:45a72715acdd5019c5be30bdbb75233e" + And user "user0" uploads chunk file "2" of "3" with "BBBBB" to "/myChecksumFile.txt" with checksum "MD5:45a72715acdd5019c5be30bdbb75233e" + And user "user0" uploads chunk file "3" of "3" with "CCCCC" to "/myChecksumFile.txt" with checksum "MD5:45a72715acdd5019c5be30bdbb75233e" When user "user0" request the checksum of "/myChecksumFile.txt" via propfind - Then The webdav checksum should match "MD5:e892fdd61a74bc89cd05673cc2e22f88" + Then The webdav checksum should match "SHA1:acfa6b1565f9710d4d497c6035d5c069bd35a8e8 MD5:45a72715acdd5019c5be30bdbb75233e ADLER32:1ecd03df" Scenario: Uploading a chunked file with checksum should return the checksum in the download header Given user "user0" exists - And user "user0" uploads chunk file "1" of "3" with "AAAAA" to "/myChecksumFile.txt" with checksum "MD5:e892fdd61a74bc89cd05673cc2e22f88" - And user "user0" uploads chunk file "2" of "3" with "BBBBB" to "/myChecksumFile.txt" with checksum "MD5:e892fdd61a74bc89cd05673cc2e22f88" - And user "user0" uploads chunk file "3" of "3" with "CCCCC" to "/myChecksumFile.txt" with checksum "MD5:e892fdd61a74bc89cd05673cc2e22f88" + And user "user0" uploads chunk file "1" of "3" with "AAAAA" to "/myChecksumFile.txt" with checksum "MD5:45a72715acdd5019c5be30bdbb75233e" + And user "user0" uploads chunk file "2" of "3" with "BBBBB" to "/myChecksumFile.txt" with checksum "MD5:45a72715acdd5019c5be30bdbb75233e" + And user "user0" uploads chunk file "3" of "3" with "CCCCC" to "/myChecksumFile.txt" with checksum "MD5:45a72715acdd5019c5be30bdbb75233e" When user "user0" downloads the file "/myChecksumFile.txt" - Then The header checksum should match "MD5:e892fdd61a74bc89cd05673cc2e22f88" + Then The header checksum should match "SHA1:acfa6b1565f9710d4d497c6035d5c069bd35a8e8 MD5:45a72715acdd5019c5be30bdbb75233e ADLER32:1ecd03df" diff --git a/tests/integration/features/webdav-related.feature b/tests/integration/features/webdav-related.feature index deb998fb06e7..2d7ba1fcd0aa 100644 --- a/tests/integration/features/webdav-related.feature +++ b/tests/integration/features/webdav-related.feature @@ -425,7 +425,7 @@ Feature: webdav-related And user "user0" uploads file with checksum "SHA1:ce5582148c6f0c1282335b87df5ed4be4b781399" and content "Some Text" to "/chksumtst.txt" When Downloading file "/chksumtst.txt" as "user0" Then The following headers should be set - | OC-Checksum | SHA1:ce5582148c6f0c1282335b87df5ed4be4b781399 | + | OC-Checksum | SHA1:ce5582148c6f0c1282335b87df5ed4be4b781399 MD5:56e57920c3c8c727bfe7a5288cdf61c4 ADLER32:1048035a | Scenario: A disabled user cannot use webdav Given user "userToBeDisabled" exists From 4dd7a11864de370a86845c845ba4c795d24a2658 Mon Sep 17 00:00:00 2001 From: Ilja Neumann Date: Mon, 13 Feb 2017 15:23:33 +0100 Subject: [PATCH 19/31] #26655 eventual mimetype error fix --- lib/private/Files/Storage/Wrapper/Checksum.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/private/Files/Storage/Wrapper/Checksum.php b/lib/private/Files/Storage/Wrapper/Checksum.php index 917df77a84db..3b3c5ffa6e07 100644 --- a/lib/private/Files/Storage/Wrapper/Checksum.php +++ b/lib/private/Files/Storage/Wrapper/Checksum.php @@ -88,10 +88,14 @@ public function getMetaData($path) { $parentMetaData['checksum'] = rtrim($checksumString); // Need to investigate more + + if (!isset($parentMetaData['mimetype'])) { - $parentMetaData['mimetype'] = ''; + $parentMetaData['mimetype'] = 'application/octet-stream'; } + + return $parentMetaData; } } \ No newline at end of file From 05e11ce17a23937f38593508f7f6f8feeef89213 Mon Sep 17 00:00:00 2001 From: Ilja Neumann Date: Tue, 14 Feb 2017 15:13:51 +0100 Subject: [PATCH 20/31] #26655 Webdav returns only one checksum (SHA1) --- apps/dav/lib/Connector/Sabre/File.php | 26 ++++++++++++++++--- apps/dav/lib/Connector/Sabre/FilesPlugin.php | 4 +-- .../Files/Storage/Wrapper/Checksum.php | 5 ---- tests/integration/features/checksums.feature | 16 ++++++------ .../features/webdav-related.feature | 2 +- 5 files changed, 33 insertions(+), 20 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php index 3140d127c526..86f84a4a87de 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -600,12 +600,30 @@ private function convertToSabreException(\Exception $e) { throw new \Sabre\DAV\Exception($e->getMessage(), 0, $e); } + /** - * Get the checksum for this file - * + * Set $algo to get a specific checksum, leave null to get all checksums + * (space seperated) + * @param null $algo * @return string */ - public function getChecksum() { - return $this->info->getChecksum(); + public function getChecksum($algo = null) { + $allChecksums = $this->info->getChecksum(); + + if (!$algo) { + return $allChecksums; + } + + $checksums = explode(' ', $allChecksums); + $algo = strtoupper($algo); + + foreach ($checksums as $checksum) { + // starts with $algo + if (substr($checksum, 0, strlen($algo)) === $algo) { + return $checksum; + } + } + + return ''; } } diff --git a/apps/dav/lib/Connector/Sabre/FilesPlugin.php b/apps/dav/lib/Connector/Sabre/FilesPlugin.php index 6d9d65ce71bc..cbe7406cc2ff 100644 --- a/apps/dav/lib/Connector/Sabre/FilesPlugin.php +++ b/apps/dav/lib/Connector/Sabre/FilesPlugin.php @@ -252,7 +252,7 @@ function httpGet(RequestInterface $request, ResponseInterface $response) { if ($node instanceof \OCA\DAV\Connector\Sabre\File) { //Add OC-Checksum header /** @var $node File */ - $checksum = $node->getChecksum(); + $checksum = $node->getChecksum('sha1'); if ($checksum !== null && $checksum !== '') { $response->addHeader('OC-Checksum', $checksum); } @@ -340,7 +340,7 @@ public function handleGetProperties(PropFind $propFind, \Sabre\DAV\INode $node) }); $propFind->handle(self::CHECKSUMS_PROPERTYNAME, function() use ($node) { - $checksum = $node->getChecksum(); + $checksum = $node->getChecksum('SHA1'); if ($checksum === NULL || $checksum === '') { return null; } diff --git a/lib/private/Files/Storage/Wrapper/Checksum.php b/lib/private/Files/Storage/Wrapper/Checksum.php index 3b3c5ffa6e07..6aaf14b34fcd 100644 --- a/lib/private/Files/Storage/Wrapper/Checksum.php +++ b/lib/private/Files/Storage/Wrapper/Checksum.php @@ -87,15 +87,10 @@ public function getMetaData($path) { $parentMetaData = $this->getWrapperStorage()->getMetaData($path); $parentMetaData['checksum'] = rtrim($checksumString); - // Need to investigate more - - if (!isset($parentMetaData['mimetype'])) { $parentMetaData['mimetype'] = 'application/octet-stream'; } - - return $parentMetaData; } } \ No newline at end of file diff --git a/tests/integration/features/checksums.feature b/tests/integration/features/checksums.feature index 303fd43bba16..1a0739df7ee0 100644 --- a/tests/integration/features/checksums.feature +++ b/tests/integration/features/checksums.feature @@ -9,41 +9,41 @@ Feature: checksums Given user "user0" exists And user "user0" uploads file "data/textfile.txt" to "/myChecksumFile.txt" with checksum "MD5:d70b40f177b14b470d1756a3c12b963a" When user "user0" request the checksum of "/myChecksumFile.txt" via propfind - Then The webdav checksum should match "SHA1:3ee962b839762adb0ad8ba6023a4690be478de6f MD5:d70b40f177b14b470d1756a3c12b963a ADLER32:8ae90960" + Then The webdav checksum should match "SHA1:3ee962b839762adb0ad8ba6023a4690be478de6f" Scenario: Uploading a file with checksum should return the checksum in the download header Given user "user0" exists And user "user0" uploads file "data/textfile.txt" to "/myChecksumFile.txt" with checksum "MD5:d70b40f177b14b470d1756a3c12b963a" When user "user0" downloads the file "/myChecksumFile.txt" - Then The header checksum should match "SHA1:3ee962b839762adb0ad8ba6023a4690be478de6f MD5:d70b40f177b14b470d1756a3c12b963a ADLER32:8ae90960" + Then The header checksum should match "SHA1:3ee962b839762adb0ad8ba6023a4690be478de6f" Scenario: Moving a file with checksum should return the checksum in the propfind Given user "user0" exists And user "user0" uploads file "data/textfile.txt" to "/myChecksumFile.txt" with checksum "MD5:d70b40f177b14b470d1756a3c12b963a" When User "user0" moved file "/myChecksumFile.txt" to "/myMovedChecksumFile.txt" And user "user0" request the checksum of "/myMovedChecksumFile.txt" via propfind - Then The webdav checksum should match "SHA1:3ee962b839762adb0ad8ba6023a4690be478de6f MD5:d70b40f177b14b470d1756a3c12b963a ADLER32:8ae90960" + Then The webdav checksum should match "SHA1:3ee962b839762adb0ad8ba6023a4690be478de6f" Scenario: Moving file with checksum should return the checksum in the download header Given user "user0" exists And user "user0" uploads file "data/textfile.txt" to "/myChecksumFile.txt" with checksum "MD5:d70b40f177b14b470d1756a3c12b963a" When User "user0" moved file "/myChecksumFile.txt" to "/myMovedChecksumFile.txt" And user "user0" downloads the file "/myMovedChecksumFile.txt" - Then The header checksum should match "SHA1:3ee962b839762adb0ad8ba6023a4690be478de6f MD5:d70b40f177b14b470d1756a3c12b963a ADLER32:8ae90960" + Then The header checksum should match "SHA1:3ee962b839762adb0ad8ba6023a4690be478de6f" Scenario: Copying a file with checksum should return the checksum in the propfind Given user "user0" exists And user "user0" uploads file "data/textfile.txt" to "/myChecksumFile.txt" with checksum "MD5:d70b40f177b14b470d1756a3c12b963a" When User "user0" copied file "/myChecksumFile.txt" to "/myChecksumFileCopy.txt" And user "user0" request the checksum of "/myChecksumFileCopy.txt" via propfind - Then The webdav checksum should match "SHA1:3ee962b839762adb0ad8ba6023a4690be478de6f MD5:d70b40f177b14b470d1756a3c12b963a ADLER32:8ae90960" + Then The webdav checksum should match "SHA1:3ee962b839762adb0ad8ba6023a4690be478de6f" Scenario: Copying file with checksum should return the checksum in the download header Given user "user0" exists And user "user0" uploads file "data/textfile.txt" to "/myChecksumFile.txt" with checksum "MD5:d70b40f177b14b470d1756a3c12b963a" When User "user0" copied file "/myChecksumFile.txt" to "/myChecksumFileCopy.txt" And user "user0" downloads the file "/myChecksumFileCopy.txt" - Then The header checksum should match "SHA1:3ee962b839762adb0ad8ba6023a4690be478de6f MD5:d70b40f177b14b470d1756a3c12b963a ADLER32:8ae90960" + Then The header checksum should match "SHA1:3ee962b839762adb0ad8ba6023a4690be478de6f" Scenario: Uploading a chunked file with checksum should return the checksum in the propfind Given user "user0" exists @@ -51,7 +51,7 @@ Feature: checksums And user "user0" uploads chunk file "2" of "3" with "BBBBB" to "/myChecksumFile.txt" with checksum "MD5:45a72715acdd5019c5be30bdbb75233e" And user "user0" uploads chunk file "3" of "3" with "CCCCC" to "/myChecksumFile.txt" with checksum "MD5:45a72715acdd5019c5be30bdbb75233e" When user "user0" request the checksum of "/myChecksumFile.txt" via propfind - Then The webdav checksum should match "SHA1:acfa6b1565f9710d4d497c6035d5c069bd35a8e8 MD5:45a72715acdd5019c5be30bdbb75233e ADLER32:1ecd03df" + Then The webdav checksum should match "SHA1:acfa6b1565f9710d4d497c6035d5c069bd35a8e8" Scenario: Uploading a chunked file with checksum should return the checksum in the download header Given user "user0" exists @@ -59,4 +59,4 @@ Feature: checksums And user "user0" uploads chunk file "2" of "3" with "BBBBB" to "/myChecksumFile.txt" with checksum "MD5:45a72715acdd5019c5be30bdbb75233e" And user "user0" uploads chunk file "3" of "3" with "CCCCC" to "/myChecksumFile.txt" with checksum "MD5:45a72715acdd5019c5be30bdbb75233e" When user "user0" downloads the file "/myChecksumFile.txt" - Then The header checksum should match "SHA1:acfa6b1565f9710d4d497c6035d5c069bd35a8e8 MD5:45a72715acdd5019c5be30bdbb75233e ADLER32:1ecd03df" + Then The header checksum should match "SHA1:acfa6b1565f9710d4d497c6035d5c069bd35a8e8" diff --git a/tests/integration/features/webdav-related.feature b/tests/integration/features/webdav-related.feature index 2d7ba1fcd0aa..deb998fb06e7 100644 --- a/tests/integration/features/webdav-related.feature +++ b/tests/integration/features/webdav-related.feature @@ -425,7 +425,7 @@ Feature: webdav-related And user "user0" uploads file with checksum "SHA1:ce5582148c6f0c1282335b87df5ed4be4b781399" and content "Some Text" to "/chksumtst.txt" When Downloading file "/chksumtst.txt" as "user0" Then The following headers should be set - | OC-Checksum | SHA1:ce5582148c6f0c1282335b87df5ed4be4b781399 MD5:56e57920c3c8c727bfe7a5288cdf61c4 ADLER32:1048035a | + | OC-Checksum | SHA1:ce5582148c6f0c1282335b87df5ed4be4b781399 | Scenario: A disabled user cannot use webdav Given user "userToBeDisabled" exists From 63495df3489c0787744769dac658ad4d376ed909 Mon Sep 17 00:00:00 2001 From: Ilja Neumann Date: Wed, 15 Feb 2017 14:02:45 +0100 Subject: [PATCH 21/31] #26655 Various smaller improvements, additional upload chunked with checksum test --- apps/dav/lib/Connector/Sabre/File.php | 20 +++++++++++------- apps/dav/lib/Connector/Sabre/FilesPlugin.php | 2 +- .../Files/Storage/Wrapper/Checksum.php | 3 ++- lib/private/Files/Stream/Checksum.php | 12 ++++++++++- .../integration/features/bootstrap/WebDav.php | 21 +++++++++++++++++++ .../features/webdav-related.feature | 10 +++++++++ 6 files changed, 57 insertions(+), 11 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php index 86f84a4a87de..a6d1efdb6d0e 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -223,10 +223,14 @@ public function put($data) { $this->refreshInfo(); - $this->fileView->putFileInfo( - $this->path, - ['checksum' => $partStorage->getMetaData($internalPartPath)['checksum']] - ); + $meta = $partStorage->getMetaData($internalPartPath); + + if (isset($meta['checksum'])) { + $this->fileView->putFileInfo( + $this->path, + ['checksum' => $meta['checksum']] + ); + } $this->refreshInfo(); @@ -533,7 +537,7 @@ private static function isChecksumValid(Storage $storage, $path) { $expectedChecksum = trim($request->server['HTTP_OC_CHECKSUM']); $computedChecksums = $meta['checksum']; - return strpos($computedChecksums, $expectedChecksum) !== false; + return strpos($computedChecksums, $expectedChecksum) == true; } @@ -615,11 +619,11 @@ public function getChecksum($algo = null) { } $checksums = explode(' ', $allChecksums); - $algo = strtoupper($algo); + $algoPrefix = strtoupper($algo) . ':'; foreach ($checksums as $checksum) { - // starts with $algo - if (substr($checksum, 0, strlen($algo)) === $algo) { + // starts with $algoPrefix + if (substr($checksum, 0, strlen($algoPrefix)) === $algoPrefix) { return $checksum; } } diff --git a/apps/dav/lib/Connector/Sabre/FilesPlugin.php b/apps/dav/lib/Connector/Sabre/FilesPlugin.php index cbe7406cc2ff..5720b6809953 100644 --- a/apps/dav/lib/Connector/Sabre/FilesPlugin.php +++ b/apps/dav/lib/Connector/Sabre/FilesPlugin.php @@ -340,7 +340,7 @@ public function handleGetProperties(PropFind $propFind, \Sabre\DAV\INode $node) }); $propFind->handle(self::CHECKSUMS_PROPERTYNAME, function() use ($node) { - $checksum = $node->getChecksum('SHA1'); + $checksum = $node->getChecksum('sha1'); if ($checksum === NULL || $checksum === '') { return null; } diff --git a/lib/private/Files/Storage/Wrapper/Checksum.php b/lib/private/Files/Storage/Wrapper/Checksum.php index 6aaf14b34fcd..99efa68462a8 100644 --- a/lib/private/Files/Storage/Wrapper/Checksum.php +++ b/lib/private/Files/Storage/Wrapper/Checksum.php @@ -55,7 +55,8 @@ public function fopen($path, $mode) { * @return bool */ private static function requiresChecksum($path, $mode) { - return substr($path, 0, 6) === 'files/' && $mode !== 'r'; + return substr($path, 0, 6) === 'files/' + && $mode !== 'r' && $mode !== 'rb'; } /** diff --git a/lib/private/Files/Stream/Checksum.php b/lib/private/Files/Stream/Checksum.php index ef564ed959d8..06157b3c0a39 100644 --- a/lib/private/Files/Stream/Checksum.php +++ b/lib/private/Files/Stream/Checksum.php @@ -35,7 +35,16 @@ */ class Checksum extends Wrapper { - /** @var resource[] */ + /** + * To stepwise compute a hash on a continuous stream + * of data a "context" is required which stores the intermediate + * hash result while the stream has not finished. + * + * After the stream ends the hashing contexts needs to be finalized + * to compute the final checksum. + * + * @var resource[] + */ private $hashingContexts; /** @var CappedMemoryCache Key is path, value is array of checksums */ @@ -113,6 +122,7 @@ public function stream_read($count) { */ public function stream_write($data) { $this->updateHashingContexts($data); + return parent::stream_write($data); } diff --git a/tests/integration/features/bootstrap/WebDav.php b/tests/integration/features/bootstrap/WebDav.php index 946a57e6f499..e3455d38eea0 100644 --- a/tests/integration/features/bootstrap/WebDav.php +++ b/tests/integration/features/bootstrap/WebDav.php @@ -590,6 +590,27 @@ public function userUploadsNewChunkFileOfWithToId($user, $num, $data, $id) $this->makeDavRequest($user, 'PUT', $destination, [], $data, "uploads"); } + /** + * @Given user :user uploads new chunk file :num with :data to id :id with checksum :checksum + */ + public function userUploadsNewChunkFileOfWithToIdWithChecksum($user, $num, $data, $id, $checksum) + { + try { + $data = \GuzzleHttp\Stream\Stream::factory($data); + $destination = '/uploads/' . $user . '/' . $id . '/' . $num; + $this->makeDavRequest( + $user, + 'PUT', + $destination, + ['OC-Checksum' => $checksum], + $data, + "uploads" + ); + } catch (\GuzzleHttp\Exception\BadResponseException $ex) { + $this->response = $ex->getResponse(); + } + } + /** * @Given user :user moves new chunk file with id :id to :dest */ diff --git a/tests/integration/features/webdav-related.feature b/tests/integration/features/webdav-related.feature index deb998fb06e7..51ff83099bb1 100644 --- a/tests/integration/features/webdav-related.feature +++ b/tests/integration/features/webdav-related.feature @@ -407,6 +407,16 @@ Feature: webdav-related And Downloading file "/myChunkedFile.txt" Then Downloaded content should be "AAAAABBBBBCCCCC" + Scenario: Upload chunked file where checksum does not match + Given using new dav path + And user "user0" exists + And user "user0" creates a new chunking upload with id "chunking-42" + And user "user0" uploads new chunk file "2" with "BBBBB" to id "chunking-42" + And user "user0" uploads new chunk file "3" with "CCCCC" to id "chunking-42" with checksum "SHA1:f005ba11" + And user "user0" uploads new chunk file "1" with "AAAAA" to id "chunking-42" + And user "user0" moves new chunk file with id "chunking-42" to "/myChunkedFile.txt" + Then the HTTP status code should be "400" + Scenario: Upload a file where checksum does not match Given user "user0" exists And file "/chksumtst.txt" does not exist for user "user0" From 6bd3aaf9e1e4dbef54211dee6e1ea772989afc08 Mon Sep 17 00:00:00 2001 From: Ilja Neumann Date: Thu, 16 Feb 2017 10:17:52 +0100 Subject: [PATCH 22/31] #26655 strpos bugfix --- apps/dav/lib/Connector/Sabre/File.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php index a6d1efdb6d0e..20ffe953b273 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -537,7 +537,7 @@ private static function isChecksumValid(Storage $storage, $path) { $expectedChecksum = trim($request->server['HTTP_OC_CHECKSUM']); $computedChecksums = $meta['checksum']; - return strpos($computedChecksums, $expectedChecksum) == true; + return strpos($computedChecksums, $expectedChecksum) === true; } From 0721f31cd6c0bc2db15a34268d257ef46d83780e Mon Sep 17 00:00:00 2001 From: Ilja Neumann Date: Fri, 17 Feb 2017 14:03:29 +0100 Subject: [PATCH 23/31] Files without checksum in oc_filecache get a checksum on first read #26655 --- .../Files/Storage/Wrapper/Checksum.php | 106 +++++++++++++++--- 1 file changed, 89 insertions(+), 17 deletions(-) diff --git a/lib/private/Files/Storage/Wrapper/Checksum.php b/lib/private/Files/Storage/Wrapper/Checksum.php index 99efa68462a8..2d1ba2e0cb7c 100644 --- a/lib/private/Files/Storage/Wrapper/Checksum.php +++ b/lib/private/Files/Storage/Wrapper/Checksum.php @@ -20,19 +20,34 @@ */ namespace OC\Files\Storage\Wrapper; +use Icewind\Streams\CallbackWrapper; +use OC\Cache\CappedMemoryCache; use OC\Files\Stream\Checksum as ChecksumStream; /** * Class Checksum * - * Computes checksums (default: SHA1) on all files under the /files path. + * Computes checksums (default: SHA1, MD5, ADLER32) on all files under the /files path. * The resulting checksum can be retrieved by call getMetadata($path) * + * If a file is read and has no checksum oc_filecache gets updated accordingly. + * + * * @package OC\Files\Storage\Wrapper */ class Checksum extends Wrapper { + const NOT_REQUIRED = 0; + /** Calculate checksum on write (to be stored in oc_filecache) */ + const PATH_NEW_OR_UPDATED = 1; + /** File needs to be checksummed on first read because it is already in cache but has no checksum */ + const PATH_IN_CACHE_WITHOUT_CHECKSUM = 2; + + + /** @var array */ + private $pathsInCacheWithoutChecksum = []; + /** * @param string $path * @param string $mode @@ -40,23 +55,86 @@ class Checksum extends Wrapper { */ public function fopen($path, $mode) { $stream = $this->getWrapperStorage()->fopen($path, $mode); - if (!self::requiresChecksum($path, $mode)) { - return $stream; + $requirement = $this->getChecksumRequirement($path, $mode); + + if ($requirement === self::PATH_NEW_OR_UPDATED) { + return \OC\Files\Stream\Checksum::wrap($stream, $path); } - return \OC\Files\Stream\Checksum::wrap($stream, $path); - } + // If file is without checksum we save the path and create + // a callback becaause we can only calculate the checksum + // after the client has read the entire filestream once. + // the checksum is then saved to oc_filecache for subsequent + // retrieval (see onClose()) + if ($requirement == self::PATH_IN_CACHE_WITHOUT_CHECKSUM) { + $this->pathsInCacheWithoutChecksum[] = $path; + $checksumStream = \OC\Files\Stream\Checksum::wrap($stream, $path); + return CallbackWrapper::wrap( + $checksumStream, + null, + null, + [$this, 'onClose'] + ); + } + return $stream; + } /** - * Checksum is only required for everything under files/ * @param $mode * @param $path - * @return bool + * @return int + */ + private function getChecksumRequirement($path, $mode) { + $isNormalFile = substr($path, 0, 6) === 'files/'; + $fileIsWritten = $mode !== 'r' && $mode !== 'rb'; + + if ($isNormalFile && $fileIsWritten) { + return self::PATH_NEW_OR_UPDATED; + } + + // file could be in cache but without checksum for example + // if mounted from ext. storage + $cache = $this->getCache($path); + + if ($cache->inCache($path)) { + $cacheEntry = $cache->get($path); + if (empty($cacheEntry['checksum'])) { + return self::PATH_IN_CACHE_WITHOUT_CHECKSUM; + } + } + + return self::NOT_REQUIRED; + } + + /** + * Callback registered in fopen */ - private static function requiresChecksum($path, $mode) { - return substr($path, 0, 6) === 'files/' - && $mode !== 'r' && $mode !== 'rb'; + public function onClose() { + $cache = $this->getCache(); + foreach ($this->pathsInCacheWithoutChecksum as $path) { + $entry = $cache->get($path); + $cache->update( + $entry->getId(), + ['checksum' => self::getChecksumsInDbFormat($path)] + ); + } + + $this->pathsInCacheWithoutChecksum = []; + } + + /** + * @param $path + * Format like "SHA1:abc MD5:def ADLER32:ghi" + * @return string + */ + private static function getChecksumsInDbFormat($path) { + $checksumString = ''; + foreach (ChecksumStream::getChecksums($path) as $algo => $checksum) { + $checksumString .= sprintf('%s:%s ', strtoupper($algo), $checksum); + } + + return rtrim($checksumString); } /** @@ -79,14 +157,8 @@ public function file_put_contents($path, $data) { * @return array */ public function getMetaData($path) { - $checksumString = ''; - - foreach (ChecksumStream::getChecksums($path) as $algo => $checksum) { - $checksumString .= sprintf('%s:%s ', strtoupper($algo), $checksum); - } - $parentMetaData = $this->getWrapperStorage()->getMetaData($path); - $parentMetaData['checksum'] = rtrim($checksumString); + $parentMetaData['checksum'] = self::getChecksumsInDbFormat($path); if (!isset($parentMetaData['mimetype'])) { $parentMetaData['mimetype'] = 'application/octet-stream'; From 2caae3e2fa3ba7fd7936a24c78c83d34eb2fae2b Mon Sep 17 00:00:00 2001 From: Ilja Neumann Date: Fri, 17 Feb 2017 14:16:54 +0100 Subject: [PATCH 24/31] Revert strpos fix #26655 --- apps/dav/lib/Connector/Sabre/File.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php index 20ffe953b273..00e95fdf14e0 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -537,7 +537,7 @@ private static function isChecksumValid(Storage $storage, $path) { $expectedChecksum = trim($request->server['HTTP_OC_CHECKSUM']); $computedChecksums = $meta['checksum']; - return strpos($computedChecksums, $expectedChecksum) === true; + return strpos($computedChecksums, $expectedChecksum) !== true; } From d8bd987292e684e0ca247f84ff3e342666374b50 Mon Sep 17 00:00:00 2001 From: Ilja Neumann Date: Wed, 22 Feb 2017 11:32:47 +0100 Subject: [PATCH 25/31] Caching of file id, and various small fixes #26655 --- .../Files/Storage/Wrapper/Checksum.php | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/lib/private/Files/Storage/Wrapper/Checksum.php b/lib/private/Files/Storage/Wrapper/Checksum.php index 2d1ba2e0cb7c..78f4da3062e3 100644 --- a/lib/private/Files/Storage/Wrapper/Checksum.php +++ b/lib/private/Files/Storage/Wrapper/Checksum.php @@ -23,6 +23,7 @@ use Icewind\Streams\CallbackWrapper; use OC\Cache\CappedMemoryCache; use OC\Files\Stream\Checksum as ChecksumStream; +use OCP\ILogger; /** * Class Checksum @@ -44,7 +45,6 @@ class Checksum extends Wrapper { /** File needs to be checksummed on first read because it is already in cache but has no checksum */ const PATH_IN_CACHE_WITHOUT_CHECKSUM = 2; - /** @var array */ private $pathsInCacheWithoutChecksum = []; @@ -62,12 +62,11 @@ public function fopen($path, $mode) { } // If file is without checksum we save the path and create - // a callback becaause we can only calculate the checksum + // a callback because we can only calculate the checksum // after the client has read the entire filestream once. // the checksum is then saved to oc_filecache for subsequent // retrieval (see onClose()) if ($requirement == self::PATH_IN_CACHE_WITHOUT_CHECKSUM) { - $this->pathsInCacheWithoutChecksum[] = $path; $checksumStream = \OC\Files\Stream\Checksum::wrap($stream, $path); return CallbackWrapper::wrap( $checksumStream, @@ -96,12 +95,11 @@ private function getChecksumRequirement($path, $mode) { // file could be in cache but without checksum for example // if mounted from ext. storage $cache = $this->getCache($path); + $cacheEntry = $cache->get($path); - if ($cache->inCache($path)) { - $cacheEntry = $cache->get($path); - if (empty($cacheEntry['checksum'])) { - return self::PATH_IN_CACHE_WITHOUT_CHECKSUM; - } + if ($cacheEntry && empty($cacheEntry['checksum'])) { + $this->pathsInCacheWithoutChecksum[$cacheEntry->getId()] = $path; + return self::PATH_IN_CACHE_WITHOUT_CHECKSUM; } return self::NOT_REQUIRED; @@ -112,10 +110,9 @@ private function getChecksumRequirement($path, $mode) { */ public function onClose() { $cache = $this->getCache(); - foreach ($this->pathsInCacheWithoutChecksum as $path) { - $entry = $cache->get($path); + foreach ($this->pathsInCacheWithoutChecksum as $cacheId => $path) { $cache->update( - $entry->getId(), + $cacheId, ['checksum' => self::getChecksumsInDbFormat($path)] ); } From 0e68f69ef38cc600a0acbd7ec8d4d19060aa7b49 Mon Sep 17 00:00:00 2001 From: Ilja Neumann Date: Wed, 22 Feb 2017 11:32:47 +0100 Subject: [PATCH 26/31] Added test about adding a file to local storage --- tests/integration/features/checksums.feature | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/integration/features/checksums.feature b/tests/integration/features/checksums.feature index 1a0739df7ee0..2010863d6bdb 100644 --- a/tests/integration/features/checksums.feature +++ b/tests/integration/features/checksums.feature @@ -60,3 +60,10 @@ Feature: checksums And user "user0" uploads chunk file "3" of "3" with "CCCCC" to "/myChecksumFile.txt" with checksum "MD5:45a72715acdd5019c5be30bdbb75233e" When user "user0" downloads the file "/myChecksumFile.txt" Then The header checksum should match "SHA1:acfa6b1565f9710d4d497c6035d5c069bd35a8e8" + + Scenario: Downloading a file from local storage has correct checksum + Given user "user0" exists + And file "prueba_cksum.txt" with text "Test file for checksums" is created in local storage + When user "user0" downloads the file "/local_storage/prueba_cksum.txt" + When user "user0" downloads the file "/local_storage/prueba_cksum.txt" + Then The header checksum should match "SHA1:b14628561796cb8bd049f036bff7948d39a0180a" From c826ea3d0ac7fc643e6ed06022428e95c635697b Mon Sep 17 00:00:00 2001 From: Sergio Bertolin Date: Tue, 7 Mar 2017 13:04:09 +0000 Subject: [PATCH 27/31] Checksum was not well calculated --- tests/integration/features/checksums.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/features/checksums.feature b/tests/integration/features/checksums.feature index 2010863d6bdb..c929e13c82ac 100644 --- a/tests/integration/features/checksums.feature +++ b/tests/integration/features/checksums.feature @@ -66,4 +66,4 @@ Feature: checksums And file "prueba_cksum.txt" with text "Test file for checksums" is created in local storage When user "user0" downloads the file "/local_storage/prueba_cksum.txt" When user "user0" downloads the file "/local_storage/prueba_cksum.txt" - Then The header checksum should match "SHA1:b14628561796cb8bd049f036bff7948d39a0180a" + Then The header checksum should match "SHA1:a35b7605c8f586d735435535c337adc066c2ccb6" From d7091b571a4edbc2ba5a3a099ee47cf2d7a859ef Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 9 Mar 2017 16:12:38 +0100 Subject: [PATCH 28/31] Don't wrap failed fopen in checksum wrapper --- lib/private/Files/Storage/Wrapper/Checksum.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/private/Files/Storage/Wrapper/Checksum.php b/lib/private/Files/Storage/Wrapper/Checksum.php index 78f4da3062e3..902cc504c8dc 100644 --- a/lib/private/Files/Storage/Wrapper/Checksum.php +++ b/lib/private/Files/Storage/Wrapper/Checksum.php @@ -55,6 +55,11 @@ class Checksum extends Wrapper { */ public function fopen($path, $mode) { $stream = $this->getWrapperStorage()->fopen($path, $mode); + if (!is_resource($stream)) { + // don't wrap on error + return $stream; + } + $requirement = $this->getChecksumRequirement($path, $mode); if ($requirement === self::PATH_NEW_OR_UPDATED) { @@ -163,4 +168,4 @@ public function getMetaData($path) { return $parentMetaData; } -} \ No newline at end of file +} From eeb8f079433f704d741e9d6b7fc43f24191da7d6 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 9 Mar 2017 16:31:51 +0100 Subject: [PATCH 29/31] Fix locking tests in ViewTest --- tests/lib/Files/ViewTest.php | 35 +++++++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/tests/lib/Files/ViewTest.php b/tests/lib/Files/ViewTest.php index fb564344942d..fb09b37c8cfd 100644 --- a/tests/lib/Files/ViewTest.php +++ b/tests/lib/Files/ViewTest.php @@ -1723,6 +1723,8 @@ public function basicOperationProviderForLocks() { ILockingProvider::LOCK_SHARED, ILockingProvider::LOCK_SHARED, null, + // shared lock stays until fclose + ILockingProvider::LOCK_SHARED, ], [ 'opendir', @@ -1797,9 +1799,12 @@ public function testLockBasicOperation( $storage->expects($this->once()) ->method($operation) ->will($this->returnCallback( - function () use ($view, $lockedPath, &$lockTypeDuring) { + function () use ($view, $lockedPath, &$lockTypeDuring, $operation) { $lockTypeDuring = $this->getFileLockType($view, $lockedPath); + if ($operation === 'fopen') { + return fopen('data://text/plain,test', 'r'); + } return true; } )); @@ -1809,7 +1814,7 @@ function () use ($view, $lockedPath, &$lockTypeDuring) { $this->connectMockHooks($hookType, $view, $lockedPath, $lockTypePre, $lockTypePost); // do operation - call_user_func_array([$view, $operation], $operationArgs); + $result = call_user_func_array([$view, $operation], $operationArgs); if ($hookType !== null) { $this->assertEquals($expectedLockBefore, $lockTypePre, 'File locked properly during pre-hook'); @@ -1820,6 +1825,13 @@ function () use ($view, $lockedPath, &$lockTypeDuring) { } $this->assertEquals($expectedStrayLock, $this->getFileLockType($view, $lockedPath)); + + if (is_resource($result)) { + fclose($result); + + // lock is cleared after fclose + $this->assertNull($this->getFileLockType($view, $lockedPath)); + } } /** @@ -2515,4 +2527,23 @@ public function testDeleteGhostFolder() { $this->assertNotEquals($rootInfo->getEtag(), $newInfo->getEtag()); $this->assertEquals(0, $newInfo->getSize()); } + + public function testFopenFail() { + // since stream wrappers influence the streams, + // this test makes sure that all stream wrappers properly return a failure + // to the caller instead of wrapping the boolean + /** @var Temporary | \PHPUnit_Framework_MockObject_MockObject $storage */ + $storage = $this->getMockBuilder(Temporary::class) + ->setMethods(['fopen']) + ->getMock(); + + $storage->expects($this->once()) + ->method('fopen') + ->willReturn(false); + + Filesystem::mount($storage, [], '/'); + $view = new View('/files'); + $result = $view->fopen('unexist.txt', 'r'); + $this->assertFalse($result); + } } From 5f18c5f9121eeb893d064dbc20fa600026d38d0d Mon Sep 17 00:00:00 2001 From: Sergio Bertolin Date: Fri, 10 Mar 2017 09:05:41 +0000 Subject: [PATCH 30/31] Checksums tests removed from webdav-feature --- .../features/webdav-related.feature | 30 ------------------- 1 file changed, 30 deletions(-) diff --git a/tests/integration/features/webdav-related.feature b/tests/integration/features/webdav-related.feature index 51ff83099bb1..fb764469592b 100644 --- a/tests/integration/features/webdav-related.feature +++ b/tests/integration/features/webdav-related.feature @@ -407,36 +407,6 @@ Feature: webdav-related And Downloading file "/myChunkedFile.txt" Then Downloaded content should be "AAAAABBBBBCCCCC" - Scenario: Upload chunked file where checksum does not match - Given using new dav path - And user "user0" exists - And user "user0" creates a new chunking upload with id "chunking-42" - And user "user0" uploads new chunk file "2" with "BBBBB" to id "chunking-42" - And user "user0" uploads new chunk file "3" with "CCCCC" to id "chunking-42" with checksum "SHA1:f005ba11" - And user "user0" uploads new chunk file "1" with "AAAAA" to id "chunking-42" - And user "user0" moves new chunk file with id "chunking-42" to "/myChunkedFile.txt" - Then the HTTP status code should be "400" - - Scenario: Upload a file where checksum does not match - Given user "user0" exists - And file "/chksumtst.txt" does not exist for user "user0" - And user "user0" uploads file with checksum "SHA1:f005ba11" and content "Some Text" to "/chksumtst.txt" - Then the HTTP status code should be "400" - - Scenario: Upload a file where checksum does match - Given user "user0" exists - And file "/chksumtst.txt" does not exist for user "user0" - And user "user0" uploads file with checksum "SHA1:ce5582148c6f0c1282335b87df5ed4be4b781399" and content "Some Text" to "/chksumtst.txt" - Then the HTTP status code should be "201" - - Scenario: Uploaded file should have the same checksum when downloaded - Given user "user0" exists - And file "/chksumtst.txt" does not exist for user "user0" - And user "user0" uploads file with checksum "SHA1:ce5582148c6f0c1282335b87df5ed4be4b781399" and content "Some Text" to "/chksumtst.txt" - When Downloading file "/chksumtst.txt" as "user0" - Then The following headers should be set - | OC-Checksum | SHA1:ce5582148c6f0c1282335b87df5ed4be4b781399 | - Scenario: A disabled user cannot use webdav Given user "userToBeDisabled" exists And As an "admin" From 0496a82cf4896e06f52eb2409f1ffe7f88f97217 Mon Sep 17 00:00:00 2001 From: Ilja Neumann Date: Fri, 10 Mar 2017 10:23:07 +0100 Subject: [PATCH 31/31] Fixed wrong strpos condition --- apps/dav/lib/Connector/Sabre/File.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php index 00e95fdf14e0..122bb69eb25f 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -537,7 +537,7 @@ private static function isChecksumValid(Storage $storage, $path) { $expectedChecksum = trim($request->server['HTTP_OC_CHECKSUM']); $computedChecksums = $meta['checksum']; - return strpos($computedChecksums, $expectedChecksum) !== true; + return strpos($computedChecksums, $expectedChecksum) !== false; }