diff options
| -rw-r--r-- | lib/private/connector/sabre/file.php | 94 | ||||
| -rw-r--r-- | tests/lib/connector/sabre/file.php | 343 |
2 files changed, 377 insertions, 60 deletions
diff --git a/lib/private/connector/sabre/file.php b/lib/private/connector/sabre/file.php index 74f96e6e1de..93244bea6ff 100644 --- a/lib/private/connector/sabre/file.php +++ b/lib/private/connector/sabre/file.php @@ -96,7 +96,11 @@ class File extends Node implements IFile { // chunked handling if (isset($_SERVER['HTTP_OC_CHUNKED'])) { - return $this->createFileChunked($data); + try { + return $this->createFileChunked($data); + } catch (\Exception $e) { + $this->convertToSabreException($e); + } } list($partStorage) = $this->fileView->resolvePath($this->path); @@ -125,7 +129,6 @@ class File extends Node implements IFile { $target = $partStorage->fopen($internalPartPath, 'wb'); if ($target === false) { \OC_Log::write('webdav', '\OC\Files\Filesystem::fopen() failed', \OC_Log::ERROR); - $partStorage->unlink($internalPartPath); // 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'); } @@ -138,32 +141,13 @@ class File extends Node implements IFile { if (isset($_SERVER['CONTENT_LENGTH']) && $_SERVER['REQUEST_METHOD'] !== 'LOCK') { $expected = $_SERVER['CONTENT_LENGTH']; if ($count != $expected) { - $partStorage->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()); - } catch (EntityTooLargeException $e) { - // the file is too big to be stored - throw new EntityTooLarge($e->getMessage()); - } catch (InvalidContentException $e) { - // the file content is not permitted - throw new UnsupportedMediaType($e->getMessage()); - } catch (InvalidPathException $e) { - // the path for the file was not valid - // TODO: find proper http status code for this case - throw new Forbidden($e->getMessage()); - } catch (LockNotAcquiredException $e) { - // the file is currently being written to by another process - throw new FileLocked($e->getMessage(), $e->getCode(), $e); - } catch (GenericEncryptionException $e) { - // 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()); + } catch (\Exception $e) { + $partStorage->unlink($internalPartPath); + $this->convertToSabreException($e); } try { @@ -192,6 +176,7 @@ class File extends Node implements IFile { try { $this->fileView->changeLock($this->path, ILockingProvider::LOCK_EXCLUSIVE); } catch (LockedException $e) { + $partStorage->unlink($internalPartPath); throw new FileLocked($e->getMessage(), $e->getCode(), $e); } @@ -207,9 +192,9 @@ class File extends Node implements IFile { $partStorage->unlink($internalPartPath); throw new Exception('Could not rename part file to final file'); } - } catch (\OCP\Files\LockNotAcquiredException $e) { - // the file is currently being written to by another process - throw new FileLocked($e->getMessage(), $e->getCode(), $e); + } catch (\Exception $e) { + $partStorage->unlink($internalPartPath); + $this->convertToSabreException($e); } } @@ -380,6 +365,9 @@ class File extends Node implements IFile { \OC_Log::write('webdav', '\OC\Files\Filesystem::rename() failed', \OC_Log::ERROR); // only delete if an error occurred and the target file was already created if ($fileExists) { + // set to null to avoid double-deletion when handling exception + // stray part file + $partFile = null; $this->fileView->unlink($targetPath); } throw new Exception('Could not rename part file assembled from chunks'); @@ -399,10 +387,11 @@ 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()); - } catch (LockedException $e) { - throw new FileLocked($e->getMessage(), $e->getCode(), $e); + } catch (\Exception $e) { + if ($partFile) { + $this->fileView->unlink($partFile); + } + $this->convertToSabreException($e); } } @@ -423,4 +412,47 @@ class File extends Node implements IFile { return !$storage->instanceOfStorage('OCA\Files_Sharing\External\Storage') && !$storage->instanceOfStorage('OC\Files\Storage\OwnCloud'); } + + /** + * Convert the given exception to a SabreException instance + * + * @param \Exception $e + * + * @throws \Sabre\DAV\Exception + */ + private function convertToSabreException(\Exception $e) { + if ($e instanceof \Sabre\DAV\Exception) { + throw $e; + } + if ($e instanceof NotPermittedException) { + // a more general case - due to whatever reason the content could not be written + throw new Forbidden($e->getMessage(), 0, $e); + } + if ($e instanceof EntityTooLargeException) { + // the file is too big to be stored + throw new EntityTooLarge($e->getMessage(), 0, $e); + } + if ($e instanceof InvalidContentException) { + // the file content is not permitted + throw new UnsupportedMediaType($e->getMessage(), 0, $e); + } + if ($e instanceof InvalidPathException) { + // the path for the file was not valid + // TODO: find proper http status code for this case + throw new Forbidden($e->getMessage(), 0, $e); + } + if ($e instanceof LockedException || $e instanceof LockNotAcquiredException) { + // the file is currently being written to by another process + throw new FileLocked($e->getMessage(), $e->getCode(), $e); + } + if ($e instanceof GenericEncryptionException) { + // returning 503 will allow retry of the operation at a later point in time + throw new ServiceUnavailable('Encryption not ready: ' . $e->getMessage(), 0, $e); + } + if ($e instanceof StorageNotAvailableException) { + throw new ServiceUnavailable('Failed to write file contents: ' . $e->getMessage(), 0, $e); + } + + throw new \Sabre\DAV\Exception($e->getMessage(), 0, $e); + } } diff --git a/tests/lib/connector/sabre/file.php b/tests/lib/connector/sabre/file.php index 834698d90cb..246f7f6d0a2 100644 --- a/tests/lib/connector/sabre/file.php +++ b/tests/lib/connector/sabre/file.php @@ -10,6 +10,7 @@ namespace Test\Connector\Sabre; use Test\HookHelper; use OC\Files\Filesystem; +use OCP\Lock\ILockingProvider; class File extends \Test\TestCase { @@ -33,6 +34,7 @@ class File extends \Test\TestCase { public function tearDown() { $userManager = \OC::$server->getUserManager(); $userManager->get($this->user)->delete(); + unset($_SERVER['HTTP_OC_CHUNKED']); parent::tearDown(); } @@ -44,23 +46,92 @@ class File extends \Test\TestCase { return $stream; } + + public function fopenFailuresProvider() { + return [ + [ + // return false + null, + '\Sabre\Dav\Exception', + false + ], + [ + new \OCP\Files\NotPermittedException(), + 'Sabre\DAV\Exception\Forbidden' + ], + [ + new \OCP\Files\EntityTooLargeException(), + 'OC\Connector\Sabre\Exception\EntityTooLarge' + ], + [ + new \OCP\Files\InvalidContentException(), + 'OC\Connector\Sabre\Exception\UnsupportedMediaType' + ], + [ + new \OCP\Files\InvalidPathException(), + 'Sabre\DAV\Exception\Forbidden' + ], + [ + new \OCP\Files\LockNotAcquiredException('/test.txt', 1), + 'OC\Connector\Sabre\Exception\FileLocked' + ], + [ + new \OCP\Lock\LockedException('/test.txt'), + 'OC\Connector\Sabre\Exception\FileLocked' + ], + [ + new \OCP\Encryption\Exceptions\GenericEncryptionException(), + 'Sabre\DAV\Exception\ServiceUnavailable' + ], + [ + new \OCP\Files\StorageNotAvailableException(), + 'Sabre\DAV\Exception\ServiceUnavailable' + ], + [ + new \Sabre\DAV\Exception('Generic sabre exception'), + 'Sabre\DAV\Exception', + false + ], + [ + new \Exception('Generic exception'), + 'Sabre\DAV\Exception' + ], + ]; + } + /** - * @expectedException \Sabre\DAV\Exception + * @dataProvider fopenFailuresProvider */ - public function testSimplePutFails() { + public function testSimplePutFails($thrownException, $expectedException, $checkPreviousClass = true) { // setup - $storage = $this->getMock('\OC\Files\Storage\Local', ['fopen'], [['datadir' => \OC::$server->getTempManager()->getTemporaryFolder()]]); + $storage = $this->getMock( + '\OC\Files\Storage\Local', + ['fopen'], + [['datadir' => \OC::$server->getTempManager()->getTemporaryFolder()]] + ); + \OC\Files\Filesystem::mount($storage, [], $this->user . '/'); $view = $this->getMock('\OC\Files\View', array('getRelativePath', 'resolvePath'), array()); - $view->expects($this->any()) + $view->expects($this->atLeastOnce()) ->method('resolvePath') - ->will($this->returnValue(array($storage, ''))); - $storage->expects($this->once()) - ->method('fopen') - ->will($this->returnValue(false)); + ->will($this->returnCallback( + function($path) use ($storage){ + return [$storage, $path]; + } + )); + + if ($thrownException !== null) { + $storage->expects($this->once()) + ->method('fopen') + ->will($this->throwException($thrownException)); + } else { + $storage->expects($this->once()) + ->method('fopen') + ->will($this->returnValue(false)); + } $view->expects($this->any()) ->method('getRelativePath') - ->will($this->returnValue('/test.txt')); + ->will($this->returnArgument(0)); $info = new \OC\Files\FileInfo('/test.txt', null, null, array( 'permissions' => \OCP\Constants::PERMISSION_ALL @@ -69,9 +140,97 @@ class File extends \Test\TestCase { $file = new \OC\Connector\Sabre\File($view, $info); // action - $file->put('test data'); + $caughtException = null; + try { + $file->put('test data'); + } catch (\Exception $e) { + $caughtException = $e; + } + + $this->assertInstanceOf($expectedException, $caughtException); + if ($checkPreviousClass) { + $this->assertInstanceOf(get_class($thrownException), $caughtException->getPrevious()); + } + + $this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files'); } + /** + * Test putting a file using chunking + * + * @dataProvider fopenFailuresProvider + */ + public function testChunkedPutFails($thrownException, $expectedException, $checkPreviousClass = false) { + // setup + $storage = $this->getMock( + '\OC\Files\Storage\Local', + ['fopen'], + [['datadir' => \OC::$server->getTempManager()->getTemporaryFolder()]] + ); + \OC\Files\Filesystem::mount($storage, [], $this->user . '/'); + $view = $this->getMock('\OC\Files\View', ['getRelativePath', 'resolvePath'], []); + $view->expects($this->atLeastOnce()) + ->method('resolvePath') + ->will($this->returnCallback( + function($path) use ($storage){ + return [$storage, $path]; + } + )); + + if ($thrownException !== null) { + $storage->expects($this->once()) + ->method('fopen') + ->will($this->throwException($thrownException)); + } else { + $storage->expects($this->once()) + ->method('fopen') + ->will($this->returnValue(false)); + } + + $view->expects($this->any()) + ->method('getRelativePath') + ->will($this->returnArgument(0)); + + $_SERVER['HTTP_OC_CHUNKED'] = true; + + $info = new \OC\Files\FileInfo('/test.txt-chunking-12345-2-0', null, null, [ + 'permissions' => \OCP\Constants::PERMISSION_ALL + ], null); + $file = new \OC\Connector\Sabre\File($view, $info); + + // put first chunk + $this->assertNull($file->put('test data one')); + + $info = new \OC\Files\FileInfo('/test.txt-chunking-12345-2-1', null, null, [ + 'permissions' => \OCP\Constants::PERMISSION_ALL + ], null); + $file = new \OC\Connector\Sabre\File($view, $info); + + // action + $caughtException = null; + try { + // last chunk + $file->put('test data two'); + } catch (\Exception $e) { + $caughtException = $e; + } + + $this->assertInstanceOf($expectedException, $caughtException); + if ($checkPreviousClass) { + $this->assertInstanceOf(get_class($thrownException), $caughtException->getPrevious()); + } + + $this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files'); + } + + /** + * Simulate putting a file to the given path. + * + * @param string $path path to put the file into + * @param string $viewRoot root to use for the view + * + * @return result of the PUT operaiton which is usually the etag + */ private function doPut($path, $viewRoot = null) { $view = \OC\Files\Filesystem::getView(); if (!is_null($viewRoot)) { @@ -90,14 +249,23 @@ class File extends \Test\TestCase { $file = new \OC\Connector\Sabre\File($view, $info); - $this->assertNotEmpty($file->put($this->getStream('test data'))); + return $file->put($this->getStream('test data')); } /** * Test putting a single file */ public function testPutSingleFile() { - $this->doPut('/foo.txt'); + $this->assertNotEmpty($this->doPut('/foo.txt')); + } + + /** + * Test putting a file using chunking + */ + public function testChunkedPut() { + $_SERVER['HTTP_OC_CHUNKED'] = true; + $this->assertNull($this->doPut('/test.txt-chunking-12345-2-0')); + $this->assertNotEmpty($this->doPut('/test.txt-chunking-12345-2-1')); } /** @@ -106,7 +274,7 @@ class File extends \Test\TestCase { public function testPutSingleFileTriggersHooks() { HookHelper::setUpHooks(); - $this->doPut('/foo.txt'); + $this->assertNotEmpty($this->doPut('/foo.txt')); $this->assertCount(4, HookHelper::$hookCalls); $this->assertHookCall( @@ -140,7 +308,7 @@ class File extends \Test\TestCase { HookHelper::setUpHooks(); - $this->doPut('/foo.txt'); + $this->assertNotEmpty($this->doPut('/foo.txt')); $this->assertCount(4, HookHelper::$hookCalls); $this->assertHookCall( @@ -177,7 +345,7 @@ class File extends \Test\TestCase { HookHelper::setUpHooks(); // happens with public webdav where the view root is the share root - $this->doPut('/foo.txt', '/' . $this->user . '/files/noderoot'); + $this->assertNotEmpty($this->doPut('/foo.txt', '/' . $this->user . '/files/noderoot')); $this->assertCount(4, HookHelper::$hookCalls); $this->assertHookCall( @@ -211,8 +379,6 @@ class File extends \Test\TestCase { /** * Test put file with cancelled hook - * - * @expectedException \Sabre\DAV\Exception */ public function testPutSingleFileCancelPreHook() { \OCP\Util::connectHook( @@ -222,13 +388,22 @@ class File extends \Test\TestCase { 'cancellingCallback' ); - $this->doPut('/foo.txt'); + // action + $thrown = false; + try { + $this->doPut('/foo.txt'); + } catch (\Sabre\DAV\Exception $e) { + $thrown = true; + } + + $this->assertTrue($thrown); + $this->assertEmpty($this->listPartFiles(), 'No stray part files'); } /** - * @expectedException \Sabre\DAV\Exception + * Test exception when the uploaded size did not match */ - public function testSimplePutFailsOnRename() { + public function testSimplePutFailsSizeCheck() { // setup $view = $this->getMock('\OC\Files\View', array('rename', 'getRelativePath', 'filesize')); @@ -238,7 +413,8 @@ class File extends \Test\TestCase { ->will($this->returnValue(false)); $view->expects($this->any()) ->method('getRelativePath') - ->will($this->returnValue('/test.txt')); + ->will($this->returnArgument(0)); + $view->expects($this->any()) ->method('filesize') ->will($this->returnValue(123456)); @@ -253,18 +429,87 @@ class File extends \Test\TestCase { $file = new \OC\Connector\Sabre\File($view, $info); // action - $file->put($this->getStream('test data')); + $thrown = false; + try { + $file->put($this->getStream('test data')); + } catch (\Sabre\DAV\Exception\BadRequest $e) { + $thrown = true; + } + + $this->assertTrue($thrown); + $this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files'); } /** - * @expectedException \OC\Connector\Sabre\Exception\InvalidPath + * Test exception during final rename in simple upload mode + */ + public function testSimplePutFailsMoveFromStorage() { + $view = new \OC\Files\View('/' . $this->user . '/files'); + + // simulate situation where the target file is locked + $view->lockFile('/test.txt', ILockingProvider::LOCK_EXCLUSIVE); + + $info = new \OC\Files\FileInfo('/' . $this->user . '/files/test.txt', null, null, array( + 'permissions' => \OCP\Constants::PERMISSION_ALL + ), null); + + $file = new \OC\Connector\Sabre\File($view, $info); + + // action + $thrown = false; + try { + $file->put($this->getStream('test data')); + } catch (\OC\Connector\Sabre\Exception\FileLocked $e) { + $thrown = true; + } + + $this->assertTrue($thrown); + $this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files'); + } + + /** + * Test exception during final rename in chunk upload mode + */ + public function testChunkedPutFailsFinalRename() { + $view = new \OC\Files\View('/' . $this->user . '/files'); + + // simulate situation where the target file is locked + $view->lockFile('/test.txt', ILockingProvider::LOCK_EXCLUSIVE); + + $_SERVER['HTTP_OC_CHUNKED'] = true; + + $info = new \OC\Files\FileInfo('/' . $this->user . '/files/test.txt-chunking-12345-2-0', null, null, [ + 'permissions' => \OCP\Constants::PERMISSION_ALL + ], null); + $file = new \OC\Connector\Sabre\File($view, $info); + $this->assertNull($file->put('test data one')); + + $info = new \OC\Files\FileInfo('/' . $this->user . '/files/test.txt-chunking-12345-2-1', null, null, [ + 'permissions' => \OCP\Constants::PERMISSION_ALL + ], null); + $file = new \OC\Connector\Sabre\File($view, $info); + + // action + $thrown = false; + try { + $file->put($this->getStream('test data')); + } catch (\OC\Connector\Sabre\Exception\FileLocked $e) { + $thrown = true; + } + + $this->assertTrue($thrown); + $this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files'); + } + + /** + * Test put file with invalid chars */ public function testSimplePutInvalidChars() { // setup $view = $this->getMock('\OC\Files\View', array('getRelativePath')); $view->expects($this->any()) ->method('getRelativePath') - ->will($this->returnValue('/*')); + ->will($this->returnArgument(0)); $info = new \OC\Files\FileInfo('/*', null, null, array( 'permissions' => \OCP\Constants::PERMISSION_ALL @@ -272,7 +517,15 @@ class File extends \Test\TestCase { $file = new \OC\Connector\Sabre\File($view, $info); // action - $file->put($this->getStream('test data')); + $thrown = false; + try { + $file->put($this->getStream('test data')); + } catch (\OC\Connector\Sabre\Exception\InvalidPath $e) { + $thrown = true; + } + + $this->assertTrue($thrown); + $this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files'); } /** @@ -286,7 +539,7 @@ class File extends \Test\TestCase { $view->expects($this->any()) ->method('getRelativePath') - ->will($this->returnValue('/*')); + ->will($this->returnArgument(0)); $info = new \OC\Files\FileInfo('/*', null, null, array( 'permissions' => \OCP\Constants::PERMISSION_ALL @@ -296,7 +549,6 @@ class File extends \Test\TestCase { } /** - * @expectedException \Sabre\DAV\Exception\BadRequest */ public function testUploadAbort() { // setup @@ -308,7 +560,7 @@ class File extends \Test\TestCase { ->will($this->returnValue(false)); $view->expects($this->any()) ->method('getRelativePath') - ->will($this->returnValue('/test.txt')); + ->will($this->returnArgument(0)); $view->expects($this->any()) ->method('filesize') ->will($this->returnValue(123456)); @@ -323,7 +575,15 @@ class File extends \Test\TestCase { $file = new \OC\Connector\Sabre\File($view, $info); // action - $file->put($this->getStream('test data')); + $thrown = false; + try { + $file->put($this->getStream('test data')); + } catch (\Sabre\DAV\Exception\BadRequest $e) { + $thrown = true; + } + + $this->assertTrue($thrown); + $this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files'); } /** @@ -484,4 +744,29 @@ class File extends \Test\TestCase { ); } + /** + * Returns part files in the given path + * + * @param \OC\Files\View view which root is the current user's "files" folder + * @param string $path path for which to list part files + * + * @return array list of part files + */ + private function listPartFiles(\OC\Files\View $userView = null, $path = '') { + if ($userView === null) { + $userView = \OC\Files\Filesystem::getView(); + } + $files = []; + list($storage, $internalPath) = $userView->resolvePath($path); + $realPath = $storage->getSourcePath($internalPath); + $dh = opendir($realPath); + while (($file = readdir($dh)) !== false) { + if (substr($file, strlen($file) - 5, 5) === '.part') { + $files[] = $file; + } + } + closedir($dh); + return $files; + } + } |