From 23eaf27a5b630cbbf0b9a664ef1945a3a5b30c31 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 23 Jun 2015 13:52:27 +0200 Subject: [PATCH 1/6] locking for chunked dav upload --- apps/dav/lib/connector/sabre/file.php | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/apps/dav/lib/connector/sabre/file.php b/apps/dav/lib/connector/sabre/file.php index 9e515cdc687..a0581de53ae 100644 --- a/apps/dav/lib/connector/sabre/file.php +++ b/apps/dav/lib/connector/sabre/file.php @@ -379,12 +379,13 @@ class File extends Node implements IFile { $this->emitPreHooks($exists, $targetPath); $this->changeLock(ILockingProvider::LOCK_EXCLUSIVE); + /** @var \OC\Files\Storage\Storage $targetStorage */ + list($targetStorage, $targetInternalPath) = $this->fileView->resolvePath($targetPath); if ($needsPartFile) { // we first assembly the target file as a part file $partFile = $path . '/' . $info['name'] . '.ocTransferId' . $info['transferid'] . '.part'; - - + /** @var \OC\Files\Storage\Storage $targetStorage */ list($partStorage, $partInternalPath) = $this->fileView->resolvePath($partFile); @@ -392,8 +393,7 @@ class File extends Node implements IFile { // here is the final atomic rename $renameOkay = $targetStorage->moveFromStorage($partStorage, $partInternalPath, $targetInternalPath); - - $fileExists = $this->fileView->file_exists($targetPath); + $fileExists = $targetStorage->file_exists($targetInternalPath); if ($renameOkay === false || $fileExists === false) { \OCP\Util::writeLog('webdav', '\OC\Files\Filesystem::rename() failed', \OCP\Util::ERROR); // only delete if an error occurred and the target file was already created @@ -427,6 +427,9 @@ class File extends Node implements IFile { $this->emitPostHooks($exists, $targetPath); $info = $this->fileView->getFileInfo($targetPath); + + $this->fileView->unlockFile($targetPath, ILockingProvider::LOCK_SHARED); + return $info->getEtag(); } catch (\Exception $e) { if ($partFile !== null) { From 21d02673becc2bc9f7312eae18c3150778a94364 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 14 Sep 2015 15:33:34 +0200 Subject: [PATCH 2/6] Add tests for uploading to locked files --- .../sabre/requesttest/uploadtest.php | 82 ++++++++++++++++++- 1 file changed, 81 insertions(+), 1 deletion(-) diff --git a/apps/dav/tests/unit/connector/sabre/requesttest/uploadtest.php b/apps/dav/tests/unit/connector/sabre/requesttest/uploadtest.php index 9a067f230a3..557e247b3b0 100644 --- a/apps/dav/tests/unit/connector/sabre/requesttest/uploadtest.php +++ b/apps/dav/tests/unit/connector/sabre/requesttest/uploadtest.php @@ -8,7 +8,9 @@ namespace OCA\DAV\Tests\Unit\Connector\Sabre\RequestTest; -use OC\AppFramework\Http; +use OC\Connector\Sabre\Exception\FileLocked; +use OCP\AppFramework\Http; +use OCP\Lock\ILockingProvider; class UploadTest extends RequestTest { public function testBasicUpload() { @@ -43,6 +45,34 @@ class UploadTest extends RequestTest { $this->assertEquals(3, $info->getSize()); } + /** + * @expectedException \OC\Connector\Sabre\Exception\FileLocked + */ + public function testUploadOverWriteReadLocked() { + $user = $this->getUniqueID(); + $view = $this->setupUser($user, 'pass'); + + $view->file_put_contents('foo.txt', 'bar'); + + $view->lockFile('/foo.txt', ILockingProvider::LOCK_SHARED); + + $this->request($view, $user, 'pass', 'PUT', '/foo.txt', 'asd'); + } + + /** + * @expectedException \OC\Connector\Sabre\Exception\FileLocked + */ + public function testUploadOverWriteWriteLocked() { + $user = $this->getUniqueID(); + $view = $this->setupUser($user, 'pass'); + + $view->file_put_contents('foo.txt', 'bar'); + + $view->lockFile('/foo.txt', ILockingProvider::LOCK_EXCLUSIVE); + + $this->request($view, $user, 'pass', 'PUT', '/foo.txt', 'asd'); + } + public function testChunkedUpload() { $user = $this->getUniqueID(); $view = $this->setupUser($user, 'pass'); @@ -107,4 +137,54 @@ class UploadTest extends RequestTest { $this->assertInstanceOf('\OC\Files\FileInfo', $info); $this->assertEquals(6, $info->getSize()); } + + /** + * @expectedException \OC\Connector\Sabre\Exception\FileLocked + */ + public function testChunkedUploadOutOfOrderReadLocked() { + $user = $this->getUniqueID(); + $view = $this->setupUser($user, 'pass'); + + $this->assertFalse($view->file_exists('foo.txt')); + + $view->lockFile('/foo.txt', ILockingProvider::LOCK_SHARED); + + try { + $response = $this->request($view, $user, 'pass', 'PUT', '/foo.txt-chunking-123-2-1', 'bar', ['OC-Chunked' => '1']); + } catch (FileLocked $e) { + $this->fail('Didn\'t expect locked error for the first chunk on read lock'); + return; + } + + $this->assertEquals(Http::STATUS_CREATED, $response->getStatus()); + $this->assertFalse($view->file_exists('foo.txt')); + + // last chunk should trigger the locked error since it tries to assemble + $this->request($view, $user, 'pass', 'PUT', '/foo.txt-chunking-123-2-0', 'asd', ['OC-Chunked' => '1']); + } + + /** + * @expectedException \OC\Connector\Sabre\Exception\FileLocked + */ + public function testChunkedUploadOutOfOrderWriteLocked() { + $user = $this->getUniqueID(); + $view = $this->setupUser($user, 'pass'); + + $this->assertFalse($view->file_exists('foo.txt')); + + $view->lockFile('/foo.txt', ILockingProvider::LOCK_EXCLUSIVE); + + try { + $response = $this->request($view, $user, 'pass', 'PUT', '/foo.txt-chunking-123-2-1', 'bar', ['OC-Chunked' => '1']); + } catch (FileLocked $e) { + $this->fail('Didn\'t expect locked error for the first chunk on write lock'); // maybe forbid this in the future for write locks only? + return; + } + + $this->assertEquals(Http::STATUS_CREATED, $response->getStatus()); + $this->assertFalse($view->file_exists('foo.txt')); + + // last chunk should trigger the locked error since it tries to assemble + $this->request($view, $user, 'pass', 'PUT', '/foo.txt-chunking-123-2-0', 'asd', ['OC-Chunked' => '1']); + } } From ddc8749814a38b33881721a8f4eccfe15e13dacb Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 15 Sep 2015 17:59:44 +0200 Subject: [PATCH 3/6] Adjust for wide locking --- apps/dav/lib/connector/sabre/file.php | 14 ++++++++++---- apps/dav/lib/connector/sabre/lockplugin.php | 4 ++-- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/apps/dav/lib/connector/sabre/file.php b/apps/dav/lib/connector/sabre/file.php index a0581de53ae..df9d98b9792 100644 --- a/apps/dav/lib/connector/sabre/file.php +++ b/apps/dav/lib/connector/sabre/file.php @@ -349,9 +349,14 @@ class File extends Node implements IFile { if (empty($info)) { throw new NotImplemented('Invalid chunk name'); } + + $this->changeLock(ILockingProvider::LOCK_EXCLUSIVE); + $chunk_handler = new \OC_FileChunking($info); $bytesWritten = $chunk_handler->store($info['index'], $data); + $this->changeLock(ILockingProvider::LOCK_SHARED); + //detect aborted upload if (isset ($_SERVER['REQUEST_METHOD']) && $_SERVER['REQUEST_METHOD'] === 'PUT') { if (isset($_SERVER['CONTENT_LENGTH'])) { @@ -376,9 +381,10 @@ class File extends Node implements IFile { $exists = $this->fileView->file_exists($targetPath); try { - $this->emitPreHooks($exists, $targetPath); + $this->fileView->lockFile($targetPath, ILockingProvider::LOCK_SHARED); - $this->changeLock(ILockingProvider::LOCK_EXCLUSIVE); + $this->emitPreHooks($exists, $targetPath); + $this->fileView->changeLock($targetPath, ILockingProvider::LOCK_EXCLUSIVE); /** @var \OC\Files\Storage\Storage $targetStorage */ list($targetStorage, $targetInternalPath) = $this->fileView->resolvePath($targetPath); @@ -403,7 +409,7 @@ class File extends Node implements IFile { $partFile = null; $targetStorage->unlink($targetInternalPath); } - $this->changeLock(ILockingProvider::LOCK_SHARED); + $this->fileView->changeLock($targetPath, ILockingProvider::LOCK_SHARED); throw new Exception('Could not rename part file assembled from chunks'); } } else { @@ -419,7 +425,7 @@ class File extends Node implements IFile { } } - $this->changeLock(ILockingProvider::LOCK_SHARED); + $this->fileView->changeLock($targetPath, ILockingProvider::LOCK_SHARED); // since we skipped the view we need to scan and emit the hooks ourselves $this->fileView->getUpdater()->update($targetPath); diff --git a/apps/dav/lib/connector/sabre/lockplugin.php b/apps/dav/lib/connector/sabre/lockplugin.php index c564e066f8e..5840e59854c 100644 --- a/apps/dav/lib/connector/sabre/lockplugin.php +++ b/apps/dav/lib/connector/sabre/lockplugin.php @@ -62,7 +62,7 @@ class LockPlugin extends ServerPlugin { public function getLock(RequestInterface $request) { // we cant listen on 'beforeMethod:PUT' due to order of operations with setting up the tree // so instead we limit ourselves to the PUT method manually - if ($request->getMethod() !== 'PUT') { + if ($request->getMethod() !== 'PUT' || isset($_SERVER['HTTP_OC_CHUNKED'])) { return; } try { @@ -80,7 +80,7 @@ class LockPlugin extends ServerPlugin { } public function releaseLock(RequestInterface $request) { - if ($request->getMethod() !== 'PUT') { + if ($request->getMethod() !== 'PUT' || isset($_SERVER['HTTP_OC_CHUNKED'])) { return; } try { From 760335c57df3338325eee0b17cabc9b887f7a5a7 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 5 Oct 2015 15:41:41 +0200 Subject: [PATCH 4/6] fix locking in tests --- apps/dav/tests/unit/connector/sabre/file.php | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/apps/dav/tests/unit/connector/sabre/file.php b/apps/dav/tests/unit/connector/sabre/file.php index 9171fc3b786..d874b7f33c2 100644 --- a/apps/dav/tests/unit/connector/sabre/file.php +++ b/apps/dav/tests/unit/connector/sabre/file.php @@ -200,7 +200,9 @@ class File extends \Test\TestCase { $file = new \OCA\DAV\Connector\Sabre\File($view, $info); // put first chunk + $file->acquireLock(ILockingProvider::LOCK_SHARED); $this->assertNull($file->put('test data one')); + $file->releaseLock(ILockingProvider::LOCK_SHARED); $info = new \OC\Files\FileInfo('/test.txt-chunking-12345-2-1', null, null, [ 'permissions' => \OCP\Constants::PERMISSION_ALL @@ -443,12 +445,12 @@ class File extends \Test\TestCase { $thrown = false; try { // beforeMethod locks - $view->lockFile('/test.txt', ILockingProvider::LOCK_SHARED); + $file->acquireLock(ILockingProvider::LOCK_SHARED); $file->put($this->getStream('test data')); // afterMethod unlocks - $view->unlockFile('/test.txt', ILockingProvider::LOCK_SHARED); + $file->releaseLock(ILockingProvider::LOCK_SHARED); } catch (\Sabre\DAV\Exception\BadRequest $e) { $thrown = true; } @@ -505,7 +507,9 @@ class File extends \Test\TestCase { 'permissions' => \OCP\Constants::PERMISSION_ALL ], null); $file = new \OCA\DAV\Connector\Sabre\File($view, $info); + $file->acquireLock(ILockingProvider::LOCK_SHARED); $this->assertNull($file->put('test data one')); + $file->releaseLock(ILockingProvider::LOCK_SHARED); $info = new \OC\Files\FileInfo('/' . $this->user . '/files/test.txt-chunking-12345-2-1', null, null, [ 'permissions' => \OCP\Constants::PERMISSION_ALL @@ -515,7 +519,9 @@ class File extends \Test\TestCase { // action $thrown = false; try { + $file->acquireLock(ILockingProvider::LOCK_SHARED); $file->put($this->getStream('test data')); + $file->releaseLock(ILockingProvider::LOCK_SHARED); } catch (\OCA\DAV\Connector\Sabre\Exception\FileLocked $e) { $thrown = true; } From 283798a2204bd19319d9ad07f9ff2e3d301a9f4a Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 7 Oct 2015 13:42:36 +0200 Subject: [PATCH 5/6] remove locking for chunks --- apps/dav/lib/connector/sabre/file.php | 4 ---- 1 file changed, 4 deletions(-) diff --git a/apps/dav/lib/connector/sabre/file.php b/apps/dav/lib/connector/sabre/file.php index df9d98b9792..961532daf50 100644 --- a/apps/dav/lib/connector/sabre/file.php +++ b/apps/dav/lib/connector/sabre/file.php @@ -350,13 +350,9 @@ class File extends Node implements IFile { throw new NotImplemented('Invalid chunk name'); } - $this->changeLock(ILockingProvider::LOCK_EXCLUSIVE); - $chunk_handler = new \OC_FileChunking($info); $bytesWritten = $chunk_handler->store($info['index'], $data); - $this->changeLock(ILockingProvider::LOCK_SHARED); - //detect aborted upload if (isset ($_SERVER['REQUEST_METHOD']) && $_SERVER['REQUEST_METHOD'] === 'PUT') { if (isset($_SERVER['CONTENT_LENGTH'])) { From 021ed8b2bd814b2ca262209738fc039a92391660 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 20 Oct 2015 16:22:45 +0200 Subject: [PATCH 6/6] adjust tests for new dav classes --- .../unit/connector/sabre/requesttest/uploadtest.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/apps/dav/tests/unit/connector/sabre/requesttest/uploadtest.php b/apps/dav/tests/unit/connector/sabre/requesttest/uploadtest.php index 557e247b3b0..a2a8326f4ff 100644 --- a/apps/dav/tests/unit/connector/sabre/requesttest/uploadtest.php +++ b/apps/dav/tests/unit/connector/sabre/requesttest/uploadtest.php @@ -46,7 +46,7 @@ class UploadTest extends RequestTest { } /** - * @expectedException \OC\Connector\Sabre\Exception\FileLocked + * @expectedException \OCA\DAV\Connector\Sabre\Exception\FileLocked */ public function testUploadOverWriteReadLocked() { $user = $this->getUniqueID(); @@ -60,7 +60,7 @@ class UploadTest extends RequestTest { } /** - * @expectedException \OC\Connector\Sabre\Exception\FileLocked + * @expectedException \OCA\DAV\Connector\Sabre\Exception\FileLocked */ public function testUploadOverWriteWriteLocked() { $user = $this->getUniqueID(); @@ -139,7 +139,7 @@ class UploadTest extends RequestTest { } /** - * @expectedException \OC\Connector\Sabre\Exception\FileLocked + * @expectedException \OCA\DAV\Connector\Sabre\Exception\FileLocked */ public function testChunkedUploadOutOfOrderReadLocked() { $user = $this->getUniqueID(); @@ -151,7 +151,7 @@ class UploadTest extends RequestTest { try { $response = $this->request($view, $user, 'pass', 'PUT', '/foo.txt-chunking-123-2-1', 'bar', ['OC-Chunked' => '1']); - } catch (FileLocked $e) { + } catch (\OCA\DAV\Connector\Sabre\Exception\FileLocked $e) { $this->fail('Didn\'t expect locked error for the first chunk on read lock'); return; } @@ -164,7 +164,7 @@ class UploadTest extends RequestTest { } /** - * @expectedException \OC\Connector\Sabre\Exception\FileLocked + * @expectedException \OCA\DAV\Connector\Sabre\Exception\FileLocked */ public function testChunkedUploadOutOfOrderWriteLocked() { $user = $this->getUniqueID(); @@ -176,7 +176,7 @@ class UploadTest extends RequestTest { try { $response = $this->request($view, $user, 'pass', 'PUT', '/foo.txt-chunking-123-2-1', 'bar', ['OC-Chunked' => '1']); - } catch (FileLocked $e) { + } catch (\OCA\DAV\Connector\Sabre\Exception\FileLocked $e) { $this->fail('Didn\'t expect locked error for the first chunk on write lock'); // maybe forbid this in the future for write locks only? return; }