From 6a5950275993394df22b208f1126efae9e35e42b Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 6 Nov 2014 16:48:20 +0100 Subject: [PATCH 1/6] Work directly on the storage when uploading over webdav --- lib/private/connector/sabre/file.php | 66 ++++++++++++++++++---------- 1 file changed, 43 insertions(+), 23 deletions(-) diff --git a/lib/private/connector/sabre/file.php b/lib/private/connector/sabre/file.php index b4acaa1507..ec27e84c19 100644 --- a/lib/private/connector/sabre/file.php +++ b/lib/private/connector/sabre/file.php @@ -83,8 +83,8 @@ class File extends Node implements IFile { */ public function put($data) { try { - if ($this->info && $this->fileView->file_exists($this->path) && - !$this->info->isUpdateable()) { + $exists = $this->fileView->file_exists($this->path); + if ($this->info && $exists && !$this->info->isUpdateable()) { throw new Forbidden(); } } catch (StorageNotAvailableException $e) { @@ -110,14 +110,31 @@ class File extends Node implements IFile { $partFilePath = $this->path; } + /** @var \OC\Files\Storage\Storage $storage */ + list($storage, $internalPartPath) = $this->fileView->resolvePath($partFilePath); + list(, $internalPath) = $this->fileView->resolvePath($this->path); try { - $putOkay = $this->fileView->file_put_contents($partFilePath, $data); - if ($putOkay === false) { + $target = $storage->fopen($internalPartPath, 'wb'); + if ($target === false) { \OC_Log::write('webdav', '\OC\Files\Filesystem::file_put_contents() failed', \OC_Log::ERROR); $this->fileView->unlink($partFilePath); // because we have no clue about the cause we can only throw back a 500/Internal Server Error throw new Exception('Could not write file contents'); } + $count = stream_copy_to_stream($data, $target); + fclose($target); + + // if content length is sent by client: + // double check if the file was fully received + // compare expected and actual size + if (isset($_SERVER['CONTENT_LENGTH']) && $_SERVER['REQUEST_METHOD'] !== 'LOCK') { + $expected = $_SERVER['CONTENT_LENGTH']; + if ($count != $expected) { + $storage->unlink($internalPartPath); + throw new BadRequest('expected filesize ' . $expected . ' got ' . $count); + } + } + } catch (NotPermittedException $e) { // a more general case - due to whatever reason the content could not be written throw new Forbidden($e->getMessage()); @@ -142,35 +159,39 @@ class File extends Node implements IFile { } try { - // if content length is sent by client: - // double check if the file was fully received - // compare expected and actual size - if (isset($_SERVER['CONTENT_LENGTH']) && $_SERVER['REQUEST_METHOD'] !== 'LOCK') { - $expected = $_SERVER['CONTENT_LENGTH']; - $actual = $this->fileView->filesize($partFilePath); - if ($actual != $expected) { - $this->fileView->unlink($partFilePath); - throw new BadRequest('expected filesize ' . $expected . ' got ' . $actual); - } - } - if ($needsPartFile) { // rename to correct path try { - $renameOkay = $this->fileView->rename($partFilePath, $this->path); - $fileExists = $this->fileView->file_exists($this->path); + $renameOkay = $storage->rename($internalPartPath, $internalPath); + $fileExists = $storage->file_exists($internalPath); if ($renameOkay === false || $fileExists === false) { \OC_Log::write('webdav', '\OC\Files\Filesystem::rename() failed', \OC_Log::ERROR); $this->fileView->unlink($partFilePath); throw new Exception('Could not rename part file to final file'); } - } - catch (LockNotAcquiredException $e) { + } catch (\OCP\Files\LockNotAcquiredException $e) { // the file is currently being written to by another process throw new FileLocked($e->getMessage(), $e->getCode(), $e); } } + // since we skipped the view we need to scan and emit the hooks ourselves + $storage->getScanner()->scanFile($internalPath); + + $hookPath = \OC\Files\Filesystem::getView()->getRelativePath($this->fileView->getAbsolutePath($this->path)); + if (!$exists) { + \OC_Hook::emit(\OC\Files\Filesystem::CLASSNAME, \OC\Files\Filesystem::signal_post_create, array( + \OC\Files\Filesystem::signal_param_path => $hookPath + )); + } else { + \OC_Hook::emit(\OC\Files\Filesystem::CLASSNAME, \OC\Files\Filesystem::signal_post_update, array( + \OC\Files\Filesystem::signal_param_path => $hookPath + )); + } + \OC_Hook::emit(\OC\Files\Filesystem::CLASSNAME, \OC\Files\Filesystem::signal_post_write, array( + \OC\Files\Filesystem::signal_param_path => $hookPath + )); + // allow sync clients to send the mtime along in a header $request = \OC::$server->getRequest(); if (isset($request->server['HTTP_X_OC_MTIME'])) { @@ -266,8 +287,7 @@ class File extends Node implements IFile { * @throws NotImplemented * @throws ServiceUnavailable */ - private function createFileChunked($data) - { + private function createFileChunked($data) { list($path, $name) = \Sabre\HTTP\URLUtil::splitPath($this->path); $info = \OC_FileChunking::decodeName($name); @@ -278,7 +298,7 @@ class File extends Node implements IFile { $bytesWritten = $chunk_handler->store($info['index'], $data); //detect aborted upload - if (isset ($_SERVER['REQUEST_METHOD']) && $_SERVER['REQUEST_METHOD'] === 'PUT' ) { + if (isset ($_SERVER['REQUEST_METHOD']) && $_SERVER['REQUEST_METHOD'] === 'PUT') { if (isset($_SERVER['CONTENT_LENGTH'])) { $expected = $_SERVER['CONTENT_LENGTH']; if ($bytesWritten != $expected) { From cbcee34eb04a9b950437f50607d8fe1b88af703a Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 9 Apr 2015 14:46:25 +0200 Subject: [PATCH 2/6] update tests --- lib/private/connector/sabre/file.php | 55 +++++++++++++++++----------- tests/lib/connector/sabre/file.php | 16 ++++++-- 2 files changed, 45 insertions(+), 26 deletions(-) diff --git a/lib/private/connector/sabre/file.php b/lib/private/connector/sabre/file.php index ec27e84c19..6483b5a61e 100644 --- a/lib/private/connector/sabre/file.php +++ b/lib/private/connector/sabre/file.php @@ -71,7 +71,7 @@ class File extends Node implements IFile { * different object on a subsequent GET you are strongly recommended to not * return an ETag, and just return null. * - * @param resource $data + * @param resource|string $data * * @throws Forbidden * @throws UnsupportedMediaType @@ -82,13 +82,18 @@ class File extends Node implements IFile { * @return string|null */ public function put($data) { + if (is_string($data)) { + $stream = fopen('php://temp', 'r+'); + fwrite($stream, $data); + $data = $stream; + }; try { $exists = $this->fileView->file_exists($this->path); if ($this->info && $exists && !$this->info->isUpdateable()) { throw new Forbidden(); } } catch (StorageNotAvailableException $e) { - throw new ServiceUnavailable("File is not updatable: ".$e->getMessage()); + throw new ServiceUnavailable("File is not updatable: " . $e->getMessage()); } // verify path of the target @@ -155,7 +160,7 @@ class File extends Node implements IFile { // returning 503 will allow retry of the operation at a later point in time throw new ServiceUnavailable("Encryption not ready: " . $e->getMessage()); } catch (StorageNotAvailableException $e) { - throw new ServiceUnavailable("Failed to write file contents: ".$e->getMessage()); + throw new ServiceUnavailable("Failed to write file contents: " . $e->getMessage()); } try { @@ -178,30 +183,33 @@ class File extends Node implements IFile { // since we skipped the view we need to scan and emit the hooks ourselves $storage->getScanner()->scanFile($internalPath); - $hookPath = \OC\Files\Filesystem::getView()->getRelativePath($this->fileView->getAbsolutePath($this->path)); - if (!$exists) { - \OC_Hook::emit(\OC\Files\Filesystem::CLASSNAME, \OC\Files\Filesystem::signal_post_create, array( - \OC\Files\Filesystem::signal_param_path => $hookPath - )); - } else { - \OC_Hook::emit(\OC\Files\Filesystem::CLASSNAME, \OC\Files\Filesystem::signal_post_update, array( + $view = \OC\Files\Filesystem::getView(); + if ($view) { + $hookPath = $view->getRelativePath($this->fileView->getAbsolutePath($this->path)); + if (!$exists) { + \OC_Hook::emit(\OC\Files\Filesystem::CLASSNAME, \OC\Files\Filesystem::signal_post_create, array( + \OC\Files\Filesystem::signal_param_path => $hookPath + )); + } else { + \OC_Hook::emit(\OC\Files\Filesystem::CLASSNAME, \OC\Files\Filesystem::signal_post_update, array( + \OC\Files\Filesystem::signal_param_path => $hookPath + )); + } + \OC_Hook::emit(\OC\Files\Filesystem::CLASSNAME, \OC\Files\Filesystem::signal_post_write, array( \OC\Files\Filesystem::signal_param_path => $hookPath )); } - \OC_Hook::emit(\OC\Files\Filesystem::CLASSNAME, \OC\Files\Filesystem::signal_post_write, array( - \OC\Files\Filesystem::signal_param_path => $hookPath - )); // allow sync clients to send the mtime along in a header $request = \OC::$server->getRequest(); if (isset($request->server['HTTP_X_OC_MTIME'])) { - if($this->fileView->touch($this->path, $request->server['HTTP_X_OC_MTIME'])) { + if ($this->fileView->touch($this->path, $request->server['HTTP_X_OC_MTIME'])) { header('X-OC-MTime: accepted'); } } $this->refreshInfo(); } catch (StorageNotAvailableException $e) { - throw new ServiceUnavailable("Failed to check file size: ".$e->getMessage()); + throw new ServiceUnavailable("Failed to check file size: " . $e->getMessage()); } return '"' . $this->info->getEtag() . '"'; @@ -209,6 +217,7 @@ class File extends Node implements IFile { /** * Returns the data + * * @return string|resource * @throws Forbidden * @throws ServiceUnavailable @@ -222,12 +231,13 @@ class File extends Node implements IFile { // returning 503 will allow retry of the operation at a later point in time throw new ServiceUnavailable("Encryption not ready: " . $e->getMessage()); } catch (StorageNotAvailableException $e) { - throw new ServiceUnavailable("Failed to open file: ".$e->getMessage()); + throw new ServiceUnavailable("Failed to open file: " . $e->getMessage()); } } /** * Delete the current file + * * @throws Forbidden * @throws ServiceUnavailable */ @@ -242,7 +252,7 @@ class File extends Node implements IFile { throw new Forbidden(); } } catch (StorageNotAvailableException $e) { - throw new ServiceUnavailable("Failed to unlink: ".$e->getMessage()); + throw new ServiceUnavailable("Failed to unlink: " . $e->getMessage()); } } @@ -257,7 +267,7 @@ class File extends Node implements IFile { $mimeType = $this->info->getMimetype(); // PROPFIND needs to return the correct mime type, for consistency with the web UI - if (isset($_SERVER['REQUEST_METHOD']) && $_SERVER['REQUEST_METHOD'] === 'PROPFIND' ) { + if (isset($_SERVER['REQUEST_METHOD']) && $_SERVER['REQUEST_METHOD'] === 'PROPFIND') { return $mimeType; } return \OC_Helper::getSecureMimeType($mimeType); @@ -310,7 +320,7 @@ class File extends Node implements IFile { } if ($chunk_handler->isComplete()) { - list($storage, ) = $this->fileView->resolvePath($path); + list($storage,) = $this->fileView->resolvePath($path); $needsPartFile = $this->needsPartFile($storage); try { @@ -339,7 +349,7 @@ class File extends Node implements IFile { // allow sync clients to send the mtime along in a header $request = \OC::$server->getRequest(); if (isset($request->server['HTTP_X_OC_MTIME'])) { - if($this->fileView->touch($targetPath, $request->server['HTTP_X_OC_MTIME'])) { + if ($this->fileView->touch($targetPath, $request->server['HTTP_X_OC_MTIME'])) { header('X-OC-MTime: accepted'); } } @@ -347,7 +357,7 @@ class File extends Node implements IFile { $info = $this->fileView->getFileInfo($targetPath); return $info->getEtag(); } catch (StorageNotAvailableException $e) { - throw new ServiceUnavailable("Failed to put file: ".$e->getMessage()); + throw new ServiceUnavailable("Failed to put file: " . $e->getMessage()); } } @@ -358,6 +368,7 @@ class File extends Node implements IFile { * Returns whether a part file is needed for the given storage * or whether the file can be assembled/uploaded directly on the * target storage. + * * @param \OCP\Files\Storage $storage * @return bool true if the storage needs part file handling */ @@ -365,6 +376,6 @@ class File extends Node implements IFile { // TODO: in the future use ChunkHandler provided by storage // and/or add method on Storage called "needsPartFile()" return !$storage->instanceOfStorage('OCA\Files_Sharing\External\Storage') && - !$storage->instanceOfStorage('OC\Files\Storage\OwnCloud'); + !$storage->instanceOfStorage('OC\Files\Storage\OwnCloud'); } } diff --git a/tests/lib/connector/sabre/file.php b/tests/lib/connector/sabre/file.php index 74e289c175..1388465631 100644 --- a/tests/lib/connector/sabre/file.php +++ b/tests/lib/connector/sabre/file.php @@ -15,9 +15,13 @@ class File extends \Test\TestCase { */ public function testSimplePutFails() { // setup - $view = $this->getMock('\OC\Files\View', array('file_put_contents', 'getRelativePath'), array()); + $storage = $this->getMock('\OC\Files\Storage\Local', ['fopen'], [['datadir' => \OC::$server->getTempManager()->getTemporaryFolder()]]); + $view = $this->getMock('\OC\Files\View', array('file_put_contents', 'getRelativePath', 'resolvePath'), array()); $view->expects($this->any()) - ->method('file_put_contents') + ->method('resolvePath') + ->will($this->returnValue(array($storage, ''))); + $storage->expects($this->once()) + ->method('fopen') ->will($this->returnValue(false)); $view->expects($this->any()) @@ -36,8 +40,9 @@ class File extends \Test\TestCase { public function testPutSingleFileShare() { // setup - $storage = $this->getMock('\OCP\Files\Storage'); - $view = $this->getMock('\OC\Files\View', array('file_put_contents', 'getRelativePath'), array()); + $stream = fopen('php://temp', 'w+'); + $storage = $this->getMock('\OC\Files\Storage\Local', ['fopen'], [['datadir' => \OC::$server->getTempManager()->getTemporaryFolder()]]); + $view = $this->getMock('\OC\Files\View', array('file_put_contents', 'getRelativePath', 'resolvePath'), array()); $view->expects($this->any()) ->method('resolvePath') ->with('') @@ -49,6 +54,9 @@ class File extends \Test\TestCase { ->method('file_put_contents') ->with('') ->will($this->returnValue(true)); + $storage->expects($this->once()) + ->method('fopen') + ->will($this->returnValue($stream)); $info = new \OC\Files\FileInfo('/foo.txt', null, null, array( 'permissions' => \OCP\Constants::PERMISSION_ALL From dcfe014103368eba463234f5369c1edbfe7a1968 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 13 Apr 2015 14:13:21 +0200 Subject: [PATCH 3/6] use our own stream copy instead --- lib/private/connector/sabre/file.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/connector/sabre/file.php b/lib/private/connector/sabre/file.php index 6483b5a61e..1e19880857 100644 --- a/lib/private/connector/sabre/file.php +++ b/lib/private/connector/sabre/file.php @@ -126,7 +126,7 @@ class File extends Node implements IFile { // because we have no clue about the cause we can only throw back a 500/Internal Server Error throw new Exception('Could not write file contents'); } - $count = stream_copy_to_stream($data, $target); + list($count, ) = \OC_Helper::streamCopy($data, $target); fclose($target); // if content length is sent by client: From 2fd44dbde44c3540d664ade0df277553e7759186 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 13 Apr 2015 14:14:48 +0200 Subject: [PATCH 4/6] rewind and update error message --- lib/private/connector/sabre/file.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/private/connector/sabre/file.php b/lib/private/connector/sabre/file.php index 1e19880857..bc4535657d 100644 --- a/lib/private/connector/sabre/file.php +++ b/lib/private/connector/sabre/file.php @@ -85,6 +85,7 @@ class File extends Node implements IFile { if (is_string($data)) { $stream = fopen('php://temp', 'r+'); fwrite($stream, $data); + fseek($stream, 0); $data = $stream; }; try { @@ -121,7 +122,7 @@ class File extends Node implements IFile { try { $target = $storage->fopen($internalPartPath, 'wb'); if ($target === false) { - \OC_Log::write('webdav', '\OC\Files\Filesystem::file_put_contents() failed', \OC_Log::ERROR); + \OC_Log::write('webdav', '\OC\Files\Filesystem::fopen() failed', \OC_Log::ERROR); $this->fileView->unlink($partFilePath); // because we have no clue about the cause we can only throw back a 500/Internal Server Error throw new Exception('Could not write file contents'); From 308af8b909822b1d1e3b601061d56d9e34585dff Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 14 Apr 2015 15:25:52 +0200 Subject: [PATCH 5/6] pass a stream to the tests --- lib/private/connector/sabre/file.php | 6 ------ tests/lib/connector/sabre/file.php | 18 +++++++++++++----- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/lib/private/connector/sabre/file.php b/lib/private/connector/sabre/file.php index bc4535657d..8d739167f8 100644 --- a/lib/private/connector/sabre/file.php +++ b/lib/private/connector/sabre/file.php @@ -82,12 +82,6 @@ class File extends Node implements IFile { * @return string|null */ public function put($data) { - if (is_string($data)) { - $stream = fopen('php://temp', 'r+'); - fwrite($stream, $data); - fseek($stream, 0); - $data = $stream; - }; try { $exists = $this->fileView->file_exists($this->path); if ($this->info && $exists && !$this->info->isUpdateable()) { diff --git a/tests/lib/connector/sabre/file.php b/tests/lib/connector/sabre/file.php index 1388465631..3fe5c2751f 100644 --- a/tests/lib/connector/sabre/file.php +++ b/tests/lib/connector/sabre/file.php @@ -10,6 +10,13 @@ namespace Test\Connector\Sabre; class File extends \Test\TestCase { + private function getStream($string) { + $stream = fopen('php://temp', 'r+'); + fwrite($stream, $string); + fseek($stream, 0); + return $stream; + } + /** * @expectedException \Sabre\DAV\Exception */ @@ -29,7 +36,7 @@ class File extends \Test\TestCase { ->will($this->returnValue('/test.txt')); $info = new \OC\Files\FileInfo('/test.txt', null, null, array( - 'permissions'=>\OCP\Constants::PERMISSION_ALL + 'permissions' => \OCP\Constants::PERMISSION_ALL ), null); $file = new \OC\Connector\Sabre\File($view, $info); @@ -64,7 +71,7 @@ class File extends \Test\TestCase { $file = new \OC\Connector\Sabre\File($view, $info); - $this->assertNotEmpty($file->put('test data')); + $this->assertNotEmpty($file->put($this->getStream('test data'))); } /** @@ -99,7 +106,7 @@ class File extends \Test\TestCase { $file = new \OC\Connector\Sabre\File($view, $info); // action - $file->put('test data'); + $file->put($this->getStream('test data')); } /** @@ -122,11 +129,12 @@ class File extends \Test\TestCase { $file = new \OC\Connector\Sabre\File($view, $info); // action - $file->put('test data'); + $file->put($this->getStream('test data')); } /** * Test setting name with setName() with invalid chars + * * @expectedException \OC\Connector\Sabre\Exception\InvalidPath */ public function testSetNameInvalidChars() { @@ -176,7 +184,7 @@ class File extends \Test\TestCase { $file = new \OC\Connector\Sabre\File($view, $info); // action - $file->put('test data'); + $file->put($this->getStream('test data')); } /** From eeecca04e66d34733e69289a314b156e8257ffd2 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 14 Apr 2015 16:25:52 +0200 Subject: [PATCH 6/6] Keep phpdoc updated. --- lib/private/connector/sabre/file.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/connector/sabre/file.php b/lib/private/connector/sabre/file.php index 8d739167f8..dc678c0894 100644 --- a/lib/private/connector/sabre/file.php +++ b/lib/private/connector/sabre/file.php @@ -71,7 +71,7 @@ class File extends Node implements IFile { * different object on a subsequent GET you are strongly recommended to not * return an ETag, and just return null. * - * @param resource|string $data + * @param resource $data * * @throws Forbidden * @throws UnsupportedMediaType