From 6fb553e92cd62926134f4f77a3069fa4439835fe Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 21 Jan 2015 22:27:59 +0100 Subject: [PATCH 01/12] Do not call wrapStorage if storate with same name added twice --- lib/private/files/filesystem.php | 5 ++++- lib/private/files/storage/storagefactory.php | 6 ++++++ lib/public/files/storage/istoragefactory.php | 2 ++ 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/lib/private/files/filesystem.php b/lib/private/files/filesystem.php index f90b2738d0..0a0c5c3612 100644 --- a/lib/private/files/filesystem.php +++ b/lib/private/files/filesystem.php @@ -175,7 +175,10 @@ class Filesystem { * @param callable $wrapper */ public static function addStorageWrapper($wrapperName, $wrapper) { - self::getLoader()->addStorageWrapper($wrapperName, $wrapper); + if (!self::getLoader()->addStorageWrapper($wrapperName, $wrapper)) { + // do not re-wrap if storage with this name already existed + return; + } $mounts = self::getMountManager()->getAll(); foreach ($mounts as $mount) { diff --git a/lib/private/files/storage/storagefactory.php b/lib/private/files/storage/storagefactory.php index c9e8d422f9..9c1c7325b7 100644 --- a/lib/private/files/storage/storagefactory.php +++ b/lib/private/files/storage/storagefactory.php @@ -23,9 +23,15 @@ class StorageFactory implements IStorageFactory { * * @param string $wrapperName * @param callable $callback + * @return true if the wrapper was added, false if there was already a wrapper with this + * name registered */ public function addStorageWrapper($wrapperName, $callback) { + if (isset($this->storageWrappers[$wrapperName])) { + return false; + } $this->storageWrappers[$wrapperName] = $callback; + return true; } /** diff --git a/lib/public/files/storage/istoragefactory.php b/lib/public/files/storage/istoragefactory.php index 769d7011de..48c12e44cd 100644 --- a/lib/public/files/storage/istoragefactory.php +++ b/lib/public/files/storage/istoragefactory.php @@ -19,6 +19,8 @@ interface IStorageFactory { * * @param string $wrapperName * @param callable $callback + * @return true if the wrapper was added, false if there was already a wrapper with this + * name registered */ public function addStorageWrapper($wrapperName, $callback); From 5fb8a4715d6ed34b1d94c5508700f3c488c0f734 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 21 Jan 2015 21:54:43 +0100 Subject: [PATCH 02/12] removeStorageWrapper to unregister a storage wrapper --- lib/private/files/filesystem.php | 10 ++++++++++ lib/private/files/storage/storagefactory.php | 17 ++++++++++++++--- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/lib/private/files/filesystem.php b/lib/private/files/filesystem.php index 0a0c5c3612..5fa3428643 100644 --- a/lib/private/files/filesystem.php +++ b/lib/private/files/filesystem.php @@ -186,6 +186,11 @@ class Filesystem { } } + /** + * Returns the storage factory + * + * @return \OCP\Files\Storage\IStorageFactory + */ public static function getLoader() { if (!self::$loader) { self::$loader = new StorageFactory(); @@ -193,6 +198,11 @@ class Filesystem { return self::$loader; } + /** + * Returns the mount manager + * + * @return \OC\Files\Filesystem\Mount\Manager + */ public static function getMountManager() { if (!self::$mounts) { \OC_Util::setupFS(); diff --git a/lib/private/files/storage/storagefactory.php b/lib/private/files/storage/storagefactory.php index 9c1c7325b7..6fa360fa84 100644 --- a/lib/private/files/storage/storagefactory.php +++ b/lib/private/files/storage/storagefactory.php @@ -21,9 +21,9 @@ class StorageFactory implements IStorageFactory { * * $callback should be a function of type (string $mountPoint, Storage $storage) => Storage * - * @param string $wrapperName - * @param callable $callback - * @return true if the wrapper was added, false if there was already a wrapper with this + * @param string $wrapperName name of the wrapper + * @param callable $callback callback + * @return bool true if the wrapper was added, false if there was already a wrapper with this * name registered */ public function addStorageWrapper($wrapperName, $callback) { @@ -34,6 +34,17 @@ class StorageFactory implements IStorageFactory { return true; } + /** + * Remove a storage wrapper by name. + * Note: internal method only to be used for cleanup + * + * @param string $wrapperName name of the wrapper + * @internal + */ + public function removeStorageWrapper($wrapperName) { + unset($this->storageWrappers[$wrapperName]); + } + /** * Create an instance of a storage and apply the registered storage wrappers * From 67f1534e0fd7c14e97fe5b17bd92aa2277520604 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 21 Jan 2015 16:29:52 +0100 Subject: [PATCH 03/12] 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 diff --git a/apps/files_encryption/tests/trashbin.php b/apps/files_encryption/tests/trashbin.php index b759c8e32f..2704a9752c 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 aa5d48b5fb..5036a260d0 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 f5cebea6b7..1833936ea2 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 0000000000..c340b9d236 --- /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, '.'))); + } +} From 960ff4f136dae689639d20a1a9f31a04e2961079 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 23 Jan 2015 13:48:35 +0100 Subject: [PATCH 04/12] Apply wrappers to existing mounts before registering it --- lib/private/files/filesystem.php | 10 +++------- lib/private/files/storage/storagefactory.php | 9 ++++++++- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/lib/private/files/filesystem.php b/lib/private/files/filesystem.php index 5fa3428643..c460159ece 100644 --- a/lib/private/files/filesystem.php +++ b/lib/private/files/filesystem.php @@ -175,15 +175,11 @@ class Filesystem { * @param callable $wrapper */ public static function addStorageWrapper($wrapperName, $wrapper) { - if (!self::getLoader()->addStorageWrapper($wrapperName, $wrapper)) { + $mounts = self::getMountManager()->getAll(); + if (!self::getLoader()->addStorageWrapper($wrapperName, $wrapper, $mounts)) { // do not re-wrap if storage with this name already existed return; } - - $mounts = self::getMountManager()->getAll(); - foreach ($mounts as $mount) { - $mount->wrapStorage($wrapper); - } } /** @@ -201,7 +197,7 @@ class Filesystem { /** * Returns the mount manager * - * @return \OC\Files\Filesystem\Mount\Manager + * @return \OC\Files\Mount\Manager */ public static function getMountManager() { if (!self::$mounts) { diff --git a/lib/private/files/storage/storagefactory.php b/lib/private/files/storage/storagefactory.php index 6fa360fa84..fa6dea2537 100644 --- a/lib/private/files/storage/storagefactory.php +++ b/lib/private/files/storage/storagefactory.php @@ -23,13 +23,20 @@ class StorageFactory implements IStorageFactory { * * @param string $wrapperName name of the wrapper * @param callable $callback callback + * @param \OCP\Files\Mount\IMountPoint[] $existingMounts existing mount points to apply the wrapper to * @return bool true if the wrapper was added, false if there was already a wrapper with this * name registered */ - public function addStorageWrapper($wrapperName, $callback) { + public function addStorageWrapper($wrapperName, $callback, $existingMounts = []) { if (isset($this->storageWrappers[$wrapperName])) { return false; } + + // apply to existing mounts before registering it to prevent applying it double in MountPoint::createStorage + foreach ($existingMounts as $mount) { + $mount->wrapStorage($callback); + } + $this->storageWrappers[$wrapperName] = $callback; return true; } From 91f3952ac1de6397057cc458f946617e90b69a18 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 23 Jan 2015 14:19:36 +0100 Subject: [PATCH 05/12] Only move files from the current user to the trashbin --- apps/files_trashbin/lib/storage.php | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/apps/files_trashbin/lib/storage.php b/apps/files_trashbin/lib/storage.php index 5036a260d0..f51d2a2fb7 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 From 2e8c70327a7d3780ee5887b172159dc0f4c76c44 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 23 Jan 2015 14:55:38 +0100 Subject: [PATCH 06/12] Remove storage wrapper for oc_trashbin in unit test --- apps/files_trashbin/tests/trashbin.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/apps/files_trashbin/tests/trashbin.php b/apps/files_trashbin/tests/trashbin.php index f572e22623..17e3801586 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(); } From 87a1b2bdc41b6a54665ded5b5ffdb9b57325177d Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 23 Jan 2015 15:11:27 +0100 Subject: [PATCH 07/12] 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(-) diff --git a/apps/files_trashbin/lib/trashbin.php b/apps/files_trashbin/lib/trashbin.php index 1833936ea2..f5cebea6b7 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 76b7d34e75..f466361bd0 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 3ff19d7385..158c964fd0 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')); + } } From 1a06edd7125f6fb71b707fa2912012b74a209a62 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 23 Jan 2015 15:22:06 +0100 Subject: [PATCH 08/12] 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(+) diff --git a/apps/files_sharing/tests/sharedstorage.php b/apps/files_sharing/tests/sharedstorage.php index 7ab1564bc3..2959b9aacf 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 1d6ec8caa6..cdaff0d0a5 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'); } /** From 1f39a7aabef951b1f737ad6dca81d848a7a8291a Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 23 Jan 2015 18:08:59 +0100 Subject: [PATCH 09/12] 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(-) diff --git a/apps/files_trashbin/tests/storage.php b/apps/files_trashbin/tests/storage.php index c340b9d236..bd58395306 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, '.'))); From a1cc9eea56a39646e679fd42b6a33249f9aad170 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 23 Jan 2015 18:39:21 +0100 Subject: [PATCH 10/12] Add trashbin storage wrapper unit test for versions --- apps/files_trashbin/tests/storage.php | 82 +++++++++++++++++++++++++-- 1 file changed, 76 insertions(+), 6 deletions(-) diff --git a/apps/files_trashbin/tests/storage.php b/apps/files_trashbin/tests/storage.php index bd58395306..050d5fcee5 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)); + } } From 02b9bad81b51a37dd60cab34b80796c79390c426 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Mon, 26 Jan 2015 12:22:22 +0100 Subject: [PATCH 11/12] 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(+) diff --git a/apps/files_trashbin/lib/storage.php b/apps/files_trashbin/lib/storage.php index f51d2a2fb7..21b4e56d0b 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 f5cebea6b7..4086bb1216 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 050d5fcee5..d9a18e5a15 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(); } From 12867b9c788487aa67ebfb4f8ee1e9997877ee69 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Tue, 27 Jan 2015 16:41:43 +0100 Subject: [PATCH 12/12] Fix return type of addStorageWrapper in PHPDoc --- lib/public/files/storage/istoragefactory.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/public/files/storage/istoragefactory.php b/lib/public/files/storage/istoragefactory.php index 48c12e44cd..50c844af2e 100644 --- a/lib/public/files/storage/istoragefactory.php +++ b/lib/public/files/storage/istoragefactory.php @@ -19,7 +19,7 @@ interface IStorageFactory { * * @param string $wrapperName * @param callable $callback - * @return true if the wrapper was added, false if there was already a wrapper with this + * @return bool true if the wrapper was added, false if there was already a wrapper with this * name registered */ public function addStorageWrapper($wrapperName, $callback);