From 67f1534e0fd7c14e97fe5b17bd92aa2277520604 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 21 Jan 2015 16:29:52 +0100 Subject: Call final unlink in trash wrapper's storage In the case of cross-storage delete, the files are copied to the trash, then deleted. The final delete on the source storage would still reach the trash wrapper. This fix makes forwards that second call to the wrapped storage to make the final delete work. It fixes the issue with remote shares, local shares and external storage. Also, it uses a new function "renameRecursive" that renames the files and preserves the mtimes (like "copy_recursive" did in the past)) --- apps/files_encryption/tests/trashbin.php | 2 + apps/files_trashbin/lib/storage.php | 3 + apps/files_trashbin/lib/trashbin.php | 43 +++++++++- apps/files_trashbin/tests/storage.php | 132 +++++++++++++++++++++++++++++++ 4 files changed, 178 insertions(+), 2 deletions(-) create mode 100644 apps/files_trashbin/tests/storage.php (limited to 'apps') diff --git a/apps/files_encryption/tests/trashbin.php b/apps/files_encryption/tests/trashbin.php index b759c8e32fd..2704a9752cc 100755 --- a/apps/files_encryption/tests/trashbin.php +++ b/apps/files_encryption/tests/trashbin.php @@ -93,6 +93,8 @@ class Trashbin extends TestCase { // cleanup test user \OC_User::deleteUser(self::TEST_ENCRYPTION_TRASHBIN_USER1); + \OC\Files\Filesystem::getLoader()->removeStorageWrapper('oc_trashbin'); + parent::tearDownAfterClass(); } diff --git a/apps/files_trashbin/lib/storage.php b/apps/files_trashbin/lib/storage.php index aa5d48b5fbe..5036a260d0c 100644 --- a/apps/files_trashbin/lib/storage.php +++ b/apps/files_trashbin/lib/storage.php @@ -46,6 +46,9 @@ class Storage extends Wrapper { if (count($parts) > 3 && $parts[2] === 'files') { $filesPath = implode('/', array_slice($parts, 3)); $result = \OCA\Files_Trashbin\Trashbin::move2trash($filesPath); + // in cross-storage cases the file will be copied + // but not deleted, so we delete it here + $this->storage->unlink($path); } else { $result = $this->storage->unlink($path); } diff --git a/apps/files_trashbin/lib/trashbin.php b/apps/files_trashbin/lib/trashbin.php index f5cebea6b78..1833936ea26 100644 --- a/apps/files_trashbin/lib/trashbin.php +++ b/apps/files_trashbin/lib/trashbin.php @@ -166,8 +166,7 @@ class Trashbin { \OC_FileProxy::$enabled = false; $trashPath = '/files_trashbin/files/' . $filename . '.d' . $timestamp; try { - $sizeOfAddedFiles = $view->filesize('/files/' . $file_path); - $view->rename('/files/' . $file_path, $trashPath); + $sizeOfAddedFiles = self::renameRecursive('/files/'.$file_path, $trashPath, $view); } catch (\OCA\Files_Trashbin\Exceptions\CopyRecursiveException $e) { $sizeOfAddedFiles = false; if ($view->file_exists($trashPath)) { @@ -806,6 +805,46 @@ class Trashbin { return $size; } + /** + * recursive rename a whole directory and preserve timestamps + * + * @param string $source source path, relative to the users files directory + * @param string $destination destination path relative to the users root directoy + * @param \OC\Files\View $view file view for the users root directory + * @return int + * @throws Exceptions\CopyRecursiveException + */ + private static function renameRecursive($source, $destination, \OC\Files\View $view) { + $size = 0; + if ($view->is_dir($source)) { + $view->mkdir($destination); + $view->touch($destination, $view->filemtime($source)); + foreach ($view->getDirectoryContent($source) as $i) { + $pathDir = $source . '/' . $i['name']; + if ($view->is_dir($pathDir)) { + $size += self::renameRecursive($pathDir, $destination . '/' . $i['name'], $view); + } else { + $size += $view->filesize($pathDir); + $mtime = $view->filemtime($pathDir); + $result = $view->rename($pathDir, $destination . '/' . $i['name']); + if (!$result) { + throw new \OCA\Files_Trashbin\Exceptions\CopyRecursiveException(); + } + $view->touch($destination . '/' . $i['name'], $mtime); + } + } + } else { + $size += $view->filesize($source); + $mtime = $view->filemtime($source); + $result = $view->rename($source, $destination); + if (!$result) { + throw new \OCA\Files_Trashbin\Exceptions\CopyRecursiveException(); + } + $view->touch($destination, $mtime); + } + return $size; + } + /** * find all versions which belong to the file we want to restore * diff --git a/apps/files_trashbin/tests/storage.php b/apps/files_trashbin/tests/storage.php new file mode 100644 index 00000000000..c340b9d2362 --- /dev/null +++ b/apps/files_trashbin/tests/storage.php @@ -0,0 +1,132 @@ + + * This file is licensed under the Affero General Public License version 3 or + * later. + * See the COPYING-README file. + */ + +namespace OCA\Files_trashbin\Tests\Storage; + +use OC\Files\Storage\Home; +use OC\Files\Storage\Temporary; +use OC\Files\Mount\MountPoint; +use OC\Files\Filesystem; + +class Storage extends \Test\TestCase { + /** + * @var \OCA\Files_trashbin\Storage + */ + private $wrapper; + + /** + * @var \OCP\Files\Storage + */ + private $storage; + + /** + * @var string + */ + private $user; + + /** + * @var \OC\Files\Storage\Storage + **/ + private $originalStorage; + + /** + * @var \OC\Files\View + */ + private $userView; + + protected function setUp() { + parent::setUp(); + + $this->user = $this->getUniqueId('user'); + \OC_User::createUser($this->user, $this->user); + + // this will setup the FS + $this->loginAsUser($this->user); + + $this->originalStorage = \OC\Files\Filesystem::getStorage('/'); + + $mockUser = $this->getMock('\OCP\IUser'); + $mockUser->expects($this->any()) + ->method('getHome') + ->will($this->returnValue($this->originalStorage->getLocalFolder($this->user))); + $mockUser->expects($this->any()) + ->method('getUID') + ->will($this->returnValue($this->user)); + + // use temp as root storage so we can wrap it for testing + $this->storage = new Home( + array('user' => $mockUser) + ); + $this->wrapper = new \OCA\Files_Trashbin\Storage( + array( + 'storage' => $this->storage, + 'mountPoint' => $this->user, + ) + ); + + // make room for a new root + Filesystem::clearMounts(); + $rootMount = new MountPoint($this->originalStorage, ''); + Filesystem::getMountManager()->addMount($rootMount); + $homeMount = new MountPoint($this->wrapper, $this->user); + Filesystem::getMountManager()->addMount($homeMount); + + $this->userView = new \OC\Files\View('/' . $this->user . '/files/'); + $this->userView->file_put_contents('test.txt', 'foo'); + } + + protected function tearDown() { + \OC\Files\Filesystem::mount($this->originalStorage, array(), '/'); + $this->logout(); + parent::tearDown(); + } + + public function testSingleStorageDelete() { + $this->assertTrue($this->storage->file_exists('files/test.txt')); + $this->userView->unlink('test.txt'); + $this->storage->getScanner()->scan(''); + $this->assertFalse($this->userView->getFileInfo('test.txt')); + $this->assertFalse($this->storage->file_exists('files/test.txt')); + + // check if file is in trashbin + $rootView = new \OC\Files\View('/'); + $results = $rootView->getDirectoryContent($this->user . '/files_trashbin/files/'); + $this->assertEquals(1, count($results)); + $name = $results[0]->getName(); + $this->assertEquals('test.txt', substr($name, 0, strrpos($name, '.'))); + } + + public function testCrossStorageDelete() { + $storage2 = new Temporary(array()); + $wrapper2 = new \OCA\Files_Trashbin\Storage( + array( + 'storage' => $storage2, + 'mountPoint' => $this->user . '/files/substorage', + ) + ); + + $mount = new MountPoint($wrapper2, $this->user . '/files/substorage'); + Filesystem::getMountManager()->addMount($mount); + + $this->userView->file_put_contents('substorage/subfile.txt', 'foo'); + $storage2->getScanner()->scan(''); + $this->assertTrue($storage2->file_exists('subfile.txt')); + $this->userView->unlink('substorage/subfile.txt'); + + $storage2->getScanner()->scan(''); + $this->assertFalse($this->userView->getFileInfo('substorage/subfile.txt')); + $this->assertFalse($storage2->file_exists('subfile.txt')); + + // check if file is in trashbin + $rootView = new \OC\Files\View('/'); + $results = $rootView->getDirectoryContent($this->user . '/files_trashbin/files'); + $this->assertEquals(1, count($results)); + $name = $results[0]->getName(); + $this->assertEquals('subfile.txt', substr($name, 0, strrpos($name, '.'))); + } +} -- cgit v1.2.3 From 91f3952ac1de6397057cc458f946617e90b69a18 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 23 Jan 2015 14:19:36 +0100 Subject: Only move files from the current user to the trashbin --- apps/files_trashbin/lib/storage.php | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) (limited to 'apps') diff --git a/apps/files_trashbin/lib/storage.php b/apps/files_trashbin/lib/storage.php index 5036a260d0c..f51d2a2fb70 100644 --- a/apps/files_trashbin/lib/storage.php +++ b/apps/files_trashbin/lib/storage.php @@ -23,6 +23,7 @@ namespace OCA\Files_Trashbin; +use OC\Files\Filesystem; use OC\Files\Storage\Wrapper\Wrapper; class Storage extends Wrapper { @@ -38,13 +39,13 @@ class Storage extends Wrapper { } public function unlink($path) { - $normalized = \OC\Files\Filesystem::normalizePath($this->mountPoint . '/' . $path); + $normalized = Filesystem::normalizePath($this->mountPoint . '/' . $path); $result = true; if (!isset($this->deletedFiles[$normalized])) { + $view = Filesystem::getView(); $this->deletedFiles[$normalized] = $normalized; - $parts = explode('/', $normalized); - if (count($parts) > 3 && $parts[2] === 'files') { - $filesPath = implode('/', array_slice($parts, 3)); + if ($filesPath = $view->getRelativePath($normalized)) { + $filesPath = trim($filesPath, '/'); $result = \OCA\Files_Trashbin\Trashbin::move2trash($filesPath); // in cross-storage cases the file will be copied // but not deleted, so we delete it here -- cgit v1.2.3 From 2e8c70327a7d3780ee5887b172159dc0f4c76c44 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 23 Jan 2015 14:55:38 +0100 Subject: Remove storage wrapper for oc_trashbin in unit test --- apps/files_trashbin/tests/trashbin.php | 2 ++ 1 file changed, 2 insertions(+) (limited to 'apps') diff --git a/apps/files_trashbin/tests/trashbin.php b/apps/files_trashbin/tests/trashbin.php index f572e22623e..17e38015868 100644 --- a/apps/files_trashbin/tests/trashbin.php +++ b/apps/files_trashbin/tests/trashbin.php @@ -88,6 +88,8 @@ class Test_Trashbin extends \Test\TestCase { \OC_Hook::clear(); + \OC\Files\Filesystem::getLoader()->removeStorageWrapper('oc_trashbin'); + parent::tearDownAfterClass(); } -- cgit v1.2.3 From 87a1b2bdc41b6a54665ded5b5ffdb9b57325177d Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 23 Jan 2015 15:11:27 +0100 Subject: Preserve mtime when doing cross storage move --- apps/files_trashbin/lib/trashbin.php | 43 ++---------------------------------- lib/private/files/view.php | 13 ++++++++--- tests/lib/files/view.php | 29 +++++++++++++++++++++++- 3 files changed, 40 insertions(+), 45 deletions(-) (limited to 'apps') diff --git a/apps/files_trashbin/lib/trashbin.php b/apps/files_trashbin/lib/trashbin.php index 1833936ea26..f5cebea6b78 100644 --- a/apps/files_trashbin/lib/trashbin.php +++ b/apps/files_trashbin/lib/trashbin.php @@ -166,7 +166,8 @@ class Trashbin { \OC_FileProxy::$enabled = false; $trashPath = '/files_trashbin/files/' . $filename . '.d' . $timestamp; try { - $sizeOfAddedFiles = self::renameRecursive('/files/'.$file_path, $trashPath, $view); + $sizeOfAddedFiles = $view->filesize('/files/' . $file_path); + $view->rename('/files/' . $file_path, $trashPath); } catch (\OCA\Files_Trashbin\Exceptions\CopyRecursiveException $e) { $sizeOfAddedFiles = false; if ($view->file_exists($trashPath)) { @@ -805,46 +806,6 @@ class Trashbin { return $size; } - /** - * recursive rename a whole directory and preserve timestamps - * - * @param string $source source path, relative to the users files directory - * @param string $destination destination path relative to the users root directoy - * @param \OC\Files\View $view file view for the users root directory - * @return int - * @throws Exceptions\CopyRecursiveException - */ - private static function renameRecursive($source, $destination, \OC\Files\View $view) { - $size = 0; - if ($view->is_dir($source)) { - $view->mkdir($destination); - $view->touch($destination, $view->filemtime($source)); - foreach ($view->getDirectoryContent($source) as $i) { - $pathDir = $source . '/' . $i['name']; - if ($view->is_dir($pathDir)) { - $size += self::renameRecursive($pathDir, $destination . '/' . $i['name'], $view); - } else { - $size += $view->filesize($pathDir); - $mtime = $view->filemtime($pathDir); - $result = $view->rename($pathDir, $destination . '/' . $i['name']); - if (!$result) { - throw new \OCA\Files_Trashbin\Exceptions\CopyRecursiveException(); - } - $view->touch($destination . '/' . $i['name'], $mtime); - } - } - } else { - $size += $view->filesize($source); - $mtime = $view->filemtime($source); - $result = $view->rename($source, $destination); - if (!$result) { - throw new \OCA\Files_Trashbin\Exceptions\CopyRecursiveException(); - } - $view->touch($destination, $mtime); - } - return $size; - } - /** * find all versions which belong to the file we want to restore * diff --git a/lib/private/files/view.php b/lib/private/files/view.php index 76b7d34e756..f466361bd02 100644 --- a/lib/private/files/view.php +++ b/lib/private/files/view.php @@ -511,7 +511,7 @@ class View { } } else { if ($this->is_dir($path1)) { - $result = $this->copy($path1, $path2); + $result = $this->copy($path1, $path2, true); if ($result === true) { $result = $storage1->rmdir($internalPath1); } @@ -519,6 +519,7 @@ class View { $source = $this->fopen($path1 . $postFix1, 'r'); $target = $this->fopen($path2 . $postFix2, 'w'); list($count, $result) = \OC_Helper::streamCopy($source, $target); + $this->touch($path2, $this->filemtime($path1)); // close open handle - especially $source is necessary because unlink below will // throw an exception on windows because the file is locked @@ -556,7 +557,7 @@ class View { } } - public function copy($path1, $path2) { + public function copy($path1, $path2, $preserveMtime = false) { $postFix1 = (substr($path1, -1, 1) === '/') ? '/' : ''; $postFix2 = (substr($path2, -1, 1) === '/') ? '/' : ''; $absolutePath1 = Filesystem::normalizePath($this->getAbsolutePath($path1)); @@ -601,10 +602,13 @@ class View { } else { if ($this->is_dir($path1) && ($dh = $this->opendir($path1))) { $result = $this->mkdir($path2); + if ($preserveMtime) { + $this->touch($path2, $this->filemtime($path1)); + } if (is_resource($dh)) { while (($file = readdir($dh)) !== false) { if (!Filesystem::isIgnoredDir($file)) { - $result = $this->copy($path1 . '/' . $file, $path2 . '/' . $file); + $result = $this->copy($path1 . '/' . $file, $path2 . '/' . $file, $preserveMtime); } } } @@ -612,6 +616,9 @@ class View { $source = $this->fopen($path1 . $postFix1, 'r'); $target = $this->fopen($path2 . $postFix2, 'w'); list($count, $result) = \OC_Helper::streamCopy($source, $target); + if($preserveMtime) { + $this->touch($path2, $this->filemtime($path1)); + } fclose($source); fclose($target); } diff --git a/tests/lib/files/view.php b/tests/lib/files/view.php index 3ff19d7385d..158c964fd0d 100644 --- a/tests/lib/files/view.php +++ b/tests/lib/files/view.php @@ -613,7 +613,7 @@ class View extends \Test\TestCase { if (\OC_Util::runningOnWindows()) { $this->markTestSkipped('[Windows] '); $depth = ((260 - $tmpdirLength) / 57); - }elseif(\OC_Util::runningOnMac()){ + } elseif (\OC_Util::runningOnMac()) { $depth = ((1024 - $tmpdirLength) / 57); } else { $depth = ((4000 - $tmpdirLength) / 57); @@ -806,4 +806,31 @@ class View extends \Test\TestCase { array('putFileInfo', array()), ); } + + public function testRenameCrossStoragePreserveMtime() { + $storage1 = new Temporary(array()); + $storage2 = new Temporary(array()); + $scanner1 = $storage1->getScanner(); + $scanner2 = $storage2->getScanner(); + $storage1->mkdir('sub'); + $storage1->mkdir('foo'); + $storage1->file_put_contents('foo.txt', 'asd'); + $storage1->file_put_contents('foo/bar.txt', 'asd'); + \OC\Files\Filesystem::mount($storage1, array(), '/test/'); + \OC\Files\Filesystem::mount($storage2, array(), '/test/sub/storage'); + + $view = new \OC\Files\View(''); + $time = time() - 200; + $view->touch('/test/foo.txt', $time); + $view->touch('/test/foo', $time); + $view->touch('/test/foo/bar.txt', $time); + + $view->rename('/test/foo.txt', '/test/sub/storage/foo.txt'); + + $this->assertEquals($time, $view->filemtime('/test/sub/storage/foo.txt')); + + $view->rename('/test/foo', '/test/sub/storage/foo'); + + $this->assertEquals($time, $view->filemtime('/test/sub/storage/foo/bar.txt')); + } } -- cgit v1.2.3 From 1a06edd7125f6fb71b707fa2912012b74a209a62 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 23 Jan 2015 15:22:06 +0100 Subject: Unregister trashbin storage wrapper at the end of tests Some more tests that uses the storage wrapper now remove it afterwards --- apps/files_sharing/tests/sharedstorage.php | 2 ++ apps/files_sharing/tests/updater.php | 2 ++ 2 files changed, 4 insertions(+) (limited to 'apps') diff --git a/apps/files_sharing/tests/sharedstorage.php b/apps/files_sharing/tests/sharedstorage.php index 7ab1564bc3d..2959b9aacfb 100644 --- a/apps/files_sharing/tests/sharedstorage.php +++ b/apps/files_sharing/tests/sharedstorage.php @@ -46,6 +46,8 @@ class Test_Files_Sharing_Storage extends OCA\Files_sharing\Tests\TestCase { $this->view->unlink($this->folder); $this->view->unlink($this->filename); + \OC\Files\Filesystem::getLoader()->removeStorageWrapper('oc_trashbin'); + parent::tearDown(); } diff --git a/apps/files_sharing/tests/updater.php b/apps/files_sharing/tests/updater.php index 1d6ec8caa61..cdaff0d0a56 100644 --- a/apps/files_sharing/tests/updater.php +++ b/apps/files_sharing/tests/updater.php @@ -111,6 +111,8 @@ class Test_Files_Sharing_Updater extends OCA\Files_sharing\Tests\TestCase { if ($status === false) { \OC_App::disable('files_trashbin'); } + + \OC\Files\Filesystem::getLoader()->removeStorageWrapper('oc_trashbin'); } /** -- cgit v1.2.3 From 1f39a7aabef951b1f737ad6dca81d848a7a8291a Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 23 Jan 2015 18:08:59 +0100 Subject: Simplify trash storage unit tests Needed to make it properly init the mount points --- apps/files_trashbin/tests/storage.php | 62 +++++++++-------------------------- 1 file changed, 16 insertions(+), 46 deletions(-) (limited to 'apps') diff --git a/apps/files_trashbin/tests/storage.php b/apps/files_trashbin/tests/storage.php index c340b9d2362..bd58395306b 100644 --- a/apps/files_trashbin/tests/storage.php +++ b/apps/files_trashbin/tests/storage.php @@ -19,11 +19,6 @@ class Storage extends \Test\TestCase { */ private $wrapper; - /** - * @var \OCP\Files\Storage - */ - private $storage; - /** * @var string */ @@ -34,6 +29,11 @@ class Storage extends \Test\TestCase { **/ private $originalStorage; + /** + * @var \OC\Files\View + */ + private $rootView; + /** * @var \OC\Files\View */ @@ -50,52 +50,31 @@ class Storage extends \Test\TestCase { $this->originalStorage = \OC\Files\Filesystem::getStorage('/'); - $mockUser = $this->getMock('\OCP\IUser'); - $mockUser->expects($this->any()) - ->method('getHome') - ->will($this->returnValue($this->originalStorage->getLocalFolder($this->user))); - $mockUser->expects($this->any()) - ->method('getUID') - ->will($this->returnValue($this->user)); - - // use temp as root storage so we can wrap it for testing - $this->storage = new Home( - array('user' => $mockUser) - ); - $this->wrapper = new \OCA\Files_Trashbin\Storage( - array( - 'storage' => $this->storage, - 'mountPoint' => $this->user, - ) - ); - - // make room for a new root - Filesystem::clearMounts(); - $rootMount = new MountPoint($this->originalStorage, ''); - Filesystem::getMountManager()->addMount($rootMount); - $homeMount = new MountPoint($this->wrapper, $this->user); - Filesystem::getMountManager()->addMount($homeMount); + \OCA\Files_Trashbin\Storage::setupStorage(); + $this->rootView = new \OC\Files\View('/'); $this->userView = new \OC\Files\View('/' . $this->user . '/files/'); $this->userView->file_put_contents('test.txt', 'foo'); + } protected function tearDown() { + \OC\Files\Filesystem::getLoader()->removeStorageWrapper('oc_trashbin'); \OC\Files\Filesystem::mount($this->originalStorage, array(), '/'); $this->logout(); + \OC_User::deleteUser($this->user); parent::tearDown(); } public function testSingleStorageDelete() { - $this->assertTrue($this->storage->file_exists('files/test.txt')); + $this->assertTrue($this->userView->file_exists('test.txt')); $this->userView->unlink('test.txt'); - $this->storage->getScanner()->scan(''); + list($storage, ) = $this->userView->resolvePath('test.txt'); + $storage->getScanner()->scan(''); // make sure we check the storage $this->assertFalse($this->userView->getFileInfo('test.txt')); - $this->assertFalse($this->storage->file_exists('files/test.txt')); // check if file is in trashbin - $rootView = new \OC\Files\View('/'); - $results = $rootView->getDirectoryContent($this->user . '/files_trashbin/files/'); + $results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files/'); $this->assertEquals(1, count($results)); $name = $results[0]->getName(); $this->assertEquals('test.txt', substr($name, 0, strrpos($name, '.'))); @@ -103,15 +82,7 @@ class Storage extends \Test\TestCase { public function testCrossStorageDelete() { $storage2 = new Temporary(array()); - $wrapper2 = new \OCA\Files_Trashbin\Storage( - array( - 'storage' => $storage2, - 'mountPoint' => $this->user . '/files/substorage', - ) - ); - - $mount = new MountPoint($wrapper2, $this->user . '/files/substorage'); - Filesystem::getMountManager()->addMount($mount); + \OC\Files\Filesystem::mount($storage2, array(), $this->user . '/files/substorage'); $this->userView->file_put_contents('substorage/subfile.txt', 'foo'); $storage2->getScanner()->scan(''); @@ -123,8 +94,7 @@ class Storage extends \Test\TestCase { $this->assertFalse($storage2->file_exists('subfile.txt')); // check if file is in trashbin - $rootView = new \OC\Files\View('/'); - $results = $rootView->getDirectoryContent($this->user . '/files_trashbin/files'); + $results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files'); $this->assertEquals(1, count($results)); $name = $results[0]->getName(); $this->assertEquals('subfile.txt', substr($name, 0, strrpos($name, '.'))); -- cgit v1.2.3 From a1cc9eea56a39646e679fd42b6a33249f9aad170 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 23 Jan 2015 18:39:21 +0100 Subject: Add trashbin storage wrapper unit test for versions --- apps/files_trashbin/tests/storage.php | 82 ++++++++++++++++++++++++++++++++--- 1 file changed, 76 insertions(+), 6 deletions(-) (limited to 'apps') diff --git a/apps/files_trashbin/tests/storage.php b/apps/files_trashbin/tests/storage.php index bd58395306b..050d5fcee5b 100644 --- a/apps/files_trashbin/tests/storage.php +++ b/apps/files_trashbin/tests/storage.php @@ -14,11 +14,6 @@ use OC\Files\Mount\MountPoint; use OC\Files\Filesystem; class Storage extends \Test\TestCase { - /** - * @var \OCA\Files_trashbin\Storage - */ - private $wrapper; - /** * @var string */ @@ -43,7 +38,7 @@ class Storage extends \Test\TestCase { parent::setUp(); $this->user = $this->getUniqueId('user'); - \OC_User::createUser($this->user, $this->user); + \OC::$server->getUserManager()->createUser($this->user, $this->user); // this will setup the FS $this->loginAsUser($this->user); @@ -66,6 +61,9 @@ class Storage extends \Test\TestCase { parent::tearDown(); } + /** + * Test that deleting a file puts it into the trashbin. + */ public function testSingleStorageDelete() { $this->assertTrue($this->userView->file_exists('test.txt')); $this->userView->unlink('test.txt'); @@ -80,6 +78,12 @@ class Storage extends \Test\TestCase { $this->assertEquals('test.txt', substr($name, 0, strrpos($name, '.'))); } + /** + * Test that deleting a file from another mounted storage properly + * lands in the trashbin. This is a cross-storage situation because + * the trashbin folder is in the root storage while the mounted one + * isn't. + */ public function testCrossStorageDelete() { $storage2 = new Temporary(array()); \OC\Files\Filesystem::mount($storage2, array(), $this->user . '/files/substorage'); @@ -99,4 +103,70 @@ class Storage extends \Test\TestCase { $name = $results[0]->getName(); $this->assertEquals('subfile.txt', substr($name, 0, strrpos($name, '.'))); } + + /** + * Test that deleted versions properly land in the trashbin. + */ + public function testDeleteVersions() { + \OCA\Files_Versions\Hooks::connectHooks(); + + // trigger a version (multiple would not work because of the expire logic) + $this->userView->file_put_contents('test.txt', 'v1'); + + $results = $this->rootView->getDirectoryContent($this->user . '/files_versions/'); + $this->assertEquals(1, count($results)); + + $this->userView->unlink('test.txt'); + + // rescan trash storage + list($rootStorage, ) = $this->rootView->resolvePath($this->user . '/files_trashbin'); + $rootStorage->getScanner()->scan(''); + + // check if versions are in trashbin + $results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/versions'); + $this->assertEquals(1, count($results)); + $name = $results[0]->getName(); + $this->assertEquals('test.txt', substr($name, 0, strlen('test.txt'))); + } + + /** + * Test that versions are not auto-trashed when moving a file between + * storages. This is because rename() between storages would call + * unlink() which should NOT trigger the version deletion logic. + */ + public function testKeepFileAndVersionsWhenMovingBetweenStorages() { + \OCA\Files_Versions\Hooks::connectHooks(); + + $storage2 = new Temporary(array()); + \OC\Files\Filesystem::mount($storage2, array(), $this->user . '/files/substorage'); + + // trigger a version (multiple would not work because of the expire logic) + $this->userView->file_put_contents('test.txt', 'v1'); + + $results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files'); + $this->assertEquals(0, count($results)); + + $results = $this->rootView->getDirectoryContent($this->user . '/files_versions/'); + $this->assertEquals(1, count($results)); + + // move to another storage + $this->userView->rename('test.txt', 'substorage/test.txt'); + $this->userView->file_exists('substorage/test.txt'); + + // rescan trash storage + list($rootStorage, ) = $this->rootView->resolvePath($this->user . '/files_trashbin'); + $rootStorage->getScanner()->scan(''); + + // versions were moved too + $results = $this->rootView->getDirectoryContent($this->user . '/files_versions/substorage'); + $this->assertEquals(1, count($results)); + + // check that nothing got trashed by the rename's unlink() call + $results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files'); + $this->assertEquals(0, count($results)); + + // check that versions were moved and not trashed + $results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/versions/'); + $this->assertEquals(0, count($results)); + } } -- cgit v1.2.3 From 02b9bad81b51a37dd60cab34b80796c79390c426 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Mon, 26 Jan 2015 12:22:22 +0100 Subject: Fix bogus deletion on copy + unlink through rename Cross-storage rename would cause copy + unlink. That unlink operation must not trigger the trashbin. --- apps/files_trashbin/lib/storage.php | 31 +++++++++++++++++++++++++++++++ apps/files_trashbin/lib/trashbin.php | 3 +++ apps/files_trashbin/tests/storage.php | 4 ++++ 3 files changed, 38 insertions(+) (limited to 'apps') diff --git a/apps/files_trashbin/lib/storage.php b/apps/files_trashbin/lib/storage.php index f51d2a2fb70..21b4e56d0bb 100644 --- a/apps/files_trashbin/lib/storage.php +++ b/apps/files_trashbin/lib/storage.php @@ -33,12 +33,43 @@ class Storage extends Wrapper { // move files across storages private $deletedFiles = array(); + /** + * Disable trash logic + * + * @var bool + */ + private static $disableTrash = false; + function __construct($parameters) { $this->mountPoint = $parameters['mountPoint']; parent::__construct($parameters); } + /** + * @internal + */ + public static function preRenameHook($params) { + // in cross-storage cases, a rename is a copy + unlink, + // that last unlink must not go to trash + self::$disableTrash = true; + } + + /** + * @internal + */ + public static function postRenameHook($params) { + self::$disableTrash = false; + } + + /** + * Deletes the given file by moving it into the trashbin. + * + * @param string $path + */ public function unlink($path) { + if (self::$disableTrash) { + return $this->storage->unlink($path); + } $normalized = Filesystem::normalizePath($this->mountPoint . '/' . $path); $result = true; if (!isset($this->deletedFiles[$normalized])) { diff --git a/apps/files_trashbin/lib/trashbin.php b/apps/files_trashbin/lib/trashbin.php index f5cebea6b78..4086bb1216d 100644 --- a/apps/files_trashbin/lib/trashbin.php +++ b/apps/files_trashbin/lib/trashbin.php @@ -928,6 +928,9 @@ class Trashbin { \OCP\Util::connectHook('OC_User', 'pre_deleteUser', 'OCA\Files_Trashbin\Hooks', 'deleteUser_hook'); //Listen to post write hook \OCP\Util::connectHook('OC_Filesystem', 'post_write', 'OCA\Files_Trashbin\Hooks', 'post_write_hook'); + // pre and post-rename, disable trash logic for the copy+unlink case + \OCP\Util::connectHook('OC_Filesystem', 'rename', 'OCA\Files_Trashbin\Storage', 'preRenameHook'); + \OCP\Util::connectHook('OC_Filesystem', 'post_rename', 'OCA\Files_Trashbin\Storage', 'postRenameHook'); } /** diff --git a/apps/files_trashbin/tests/storage.php b/apps/files_trashbin/tests/storage.php index 050d5fcee5b..d9a18e5a15c 100644 --- a/apps/files_trashbin/tests/storage.php +++ b/apps/files_trashbin/tests/storage.php @@ -37,6 +37,9 @@ class Storage extends \Test\TestCase { protected function setUp() { parent::setUp(); + \OC_Hook::clear(); + \OCA\Files_Trashbin\Trashbin::registerHooks(); + $this->user = $this->getUniqueId('user'); \OC::$server->getUserManager()->createUser($this->user, $this->user); @@ -58,6 +61,7 @@ class Storage extends \Test\TestCase { \OC\Files\Filesystem::mount($this->originalStorage, array(), '/'); $this->logout(); \OC_User::deleteUser($this->user); + \OC_Hook::clear(); parent::tearDown(); } -- cgit v1.2.3