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_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'); } /** diff --git a/apps/files_trashbin/lib/storage.php b/apps/files_trashbin/lib/storage.php index aa5d48b5fb..21b4e56d0b 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 { @@ -32,20 +33,54 @@ 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) { - $normalized = \OC\Files\Filesystem::normalizePath($this->mountPoint . '/' . $path); + if (self::$disableTrash) { + return $this->storage->unlink($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 + $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..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 new file mode 100644 index 0000000000..d9a18e5a15 --- /dev/null +++ b/apps/files_trashbin/tests/storage.php @@ -0,0 +1,176 @@ + + * 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 string + */ + private $user; + + /** + * @var \OC\Files\Storage\Storage + **/ + private $originalStorage; + + /** + * @var \OC\Files\View + */ + private $rootView; + + /** + * @var \OC\Files\View + */ + private $userView; + + 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); + + // this will setup the FS + $this->loginAsUser($this->user); + + $this->originalStorage = \OC\Files\Filesystem::getStorage('/'); + + \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); + \OC_Hook::clear(); + 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'); + list($storage, ) = $this->userView->resolvePath('test.txt'); + $storage->getScanner()->scan(''); // make sure we check the storage + $this->assertFalse($this->userView->getFileInfo('test.txt')); + + // check if file is in trashbin + $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, '.'))); + } + + /** + * 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'); + + $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 + $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, '.'))); + } + + /** + * 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)); + } +} 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(); } diff --git a/lib/private/files/filesystem.php b/lib/private/files/filesystem.php index f90b2738d0..c460159ece 100644 --- a/lib/private/files/filesystem.php +++ b/lib/private/files/filesystem.php @@ -175,14 +175,18 @@ class Filesystem { * @param callable $wrapper */ public static function addStorageWrapper($wrapperName, $wrapper) { - self::getLoader()->addStorageWrapper($wrapperName, $wrapper); - $mounts = self::getMountManager()->getAll(); - foreach ($mounts as $mount) { - $mount->wrapStorage($wrapper); + if (!self::getLoader()->addStorageWrapper($wrapperName, $wrapper, $mounts)) { + // do not re-wrap if storage with this name already existed + return; } } + /** + * Returns the storage factory + * + * @return \OCP\Files\Storage\IStorageFactory + */ public static function getLoader() { if (!self::$loader) { self::$loader = new StorageFactory(); @@ -190,6 +194,11 @@ class Filesystem { return self::$loader; } + /** + * Returns the mount manager + * + * @return \OC\Files\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 c9e8d422f9..fa6dea2537 100644 --- a/lib/private/files/storage/storagefactory.php +++ b/lib/private/files/storage/storagefactory.php @@ -21,11 +21,35 @@ class StorageFactory implements IStorageFactory { * * $callback should be a function of type (string $mountPoint, Storage $storage) => Storage * - * @param string $wrapperName - * @param callable $callback + * @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; + } + + /** + * 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]); } /** 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/lib/public/files/storage/istoragefactory.php b/lib/public/files/storage/istoragefactory.php index 769d7011de..50c844af2e 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 bool true if the wrapper was added, false if there was already a wrapper with this + * name registered */ public function addStorageWrapper($wrapperName, $callback); 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')); + } }