From 3b9fa8158183950634172799737489cefccd170c Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Wed, 2 Jul 2014 14:40:22 +0200 Subject: [PATCH 1/3] if a folder gets deleted we unshare all shared files/folders below --- apps/files_sharing/lib/proxy.php | 24 ++++++++++------------- apps/files_sharing/lib/sharedmount.php | 2 ++ apps/files_sharing/tests/proxy.php | 27 +++++++++++++------------- apps/files_sharing/tests/updater.php | 14 ++++++------- 4 files changed, 33 insertions(+), 34 deletions(-) diff --git a/apps/files_sharing/lib/proxy.php b/apps/files_sharing/lib/proxy.php index f595328cc6..92303d298b 100644 --- a/apps/files_sharing/lib/proxy.php +++ b/apps/files_sharing/lib/proxy.php @@ -21,47 +21,43 @@ */ namespace OCA\Files\Share; +use OCA\Files_Sharing\Helper; class Proxy extends \OC_FileProxy { /** - * check if the deleted folder contains share mount points and move them - * up to the parent + * check if the deleted folder contains share mount points and unshare them * * @param string $path */ public function preUnlink($path) { - $this->moveMountPointsUp($path); + $this->unshareChildren($path); } /** - * check if the deleted folder contains share mount points and move them - * up to the parent + * check if the deleted folder contains share mount points and unshare them * * @param string $path */ public function preRmdir($path) { - $this->moveMountPointsUp($path); + $this->unshareChildren($path); } /** - * move share mount points up to the parent + * unshare shared items below the deleted folder * * @param string $path */ - private function moveMountPointsUp($path) { + private function unshareChildren($path) { $view = new \OC\Files\View('/'); - // find share mount points within $path and move them up to the parent folder - // before we delete $path + // find share mount points within $path and unmount them $mountManager = \OC\Files\Filesystem::getMountManager(); $mountedShares = $mountManager->findIn($path); foreach ($mountedShares as $mount) { - if ($mount->getStorage()->instanceOfStorage('\OC\Files\Storage\Shared')) { + if ($mount->getStorage()->instanceOfStorage('OCA\Files_Sharing\ISharedStorage')) { $mountPoint = $mount->getMountPoint(); - $mountPointName = $mount->getMountPointName(); - $target = \OCA\Files_Sharing\Helper::generateUniqueTarget(dirname($path) . '/' . $mountPointName, array(), $view); - $view->rename($mountPoint, $target); + $view->unlink($mountPoint); } } } diff --git a/apps/files_sharing/lib/sharedmount.php b/apps/files_sharing/lib/sharedmount.php index f8def2c6a8..564ac43ec7 100644 --- a/apps/files_sharing/lib/sharedmount.php +++ b/apps/files_sharing/lib/sharedmount.php @@ -143,8 +143,10 @@ class SharedMount extends Mount implements MoveableMount { * @return bool */ public function removeMount() { + $mountManager = \OC\Files\Filesystem::getMountManager(); $storage = $this->getStorage(); $result = \OCP\Share::unshareFromSelf($storage->getItemType(), $storage->getMountPoint()); + $mountManager->removeMount($this->mountPoint); return $result; } diff --git a/apps/files_sharing/tests/proxy.php b/apps/files_sharing/tests/proxy.php index 634ed86db5..b6599a1b64 100644 --- a/apps/files_sharing/tests/proxy.php +++ b/apps/files_sharing/tests/proxy.php @@ -47,7 +47,6 @@ class Test_Files_Sharing_Proxy extends Test_Files_Sharing_Base { $this->filename = '/share-api-test'; // save file with content - $this->view->file_put_contents($this->filename, $this->data); $this->view->mkdir($this->folder); $this->view->mkdir($this->folder . $this->subfolder); $this->view->mkdir($this->folder . $this->subfolder . $this->subsubfolder); @@ -56,7 +55,6 @@ class Test_Files_Sharing_Proxy extends Test_Files_Sharing_Base { } function tearDown() { - $this->view->unlink($this->filename); $this->view->deleteAll($this->folder); self::$tempStorage = null; @@ -69,30 +67,33 @@ class Test_Files_Sharing_Proxy extends Test_Files_Sharing_Base { */ function testpreUnlink() { - $fileInfo1 = \OC\Files\Filesystem::getFileInfo($this->filename); $fileInfo2 = \OC\Files\Filesystem::getFileInfo($this->folder); - $result = \OCP\Share::shareItem('file', $fileInfo1->getId(), \OCP\Share::SHARE_TYPE_USER, self::TEST_FILES_SHARING_API_USER2, 31); - $this->assertTrue($result); - $result = \OCP\Share::shareItem('folder', $fileInfo2->getId(), \OCP\Share::SHARE_TYPE_USER, self::TEST_FILES_SHARING_API_USER2, 31); $this->assertTrue($result); self::loginHelper(self::TEST_FILES_SHARING_API_USER2); - // move shared folder to 'localDir' and rename it, so that it uses the same - // name as the shared file + // one folder should be shared with the user + $sharedFolders = \OCP\Share::getItemsSharedWith('folder'); + $this->assertSame(1, count($sharedFolders)); + + // move shared folder to 'localDir' \OC\Files\Filesystem::mkdir('localDir'); - $result = \OC\Files\Filesystem::rename($this->folder, '/localDir/' . $this->filename); + $result = \OC\Files\Filesystem::rename($this->folder, '/localDir/' . $this->folder); $this->assertTrue($result); \OC\Files\Filesystem::unlink('localDir'); self::loginHelper(self::TEST_FILES_SHARING_API_USER2); - // after we deleted 'localDir' the share should be moved up to the root and be - // renamed to "filename (2)" - $this->assertTrue(\OC\Files\Filesystem::file_exists($this->filename)); - $this->assertTrue(\OC\Files\Filesystem::file_exists($this->filename . ' (2)' )); + // after the parent directory was deleted the share should be unshared + $sharedFolders = \OCP\Share::getItemsSharedWith('folder'); + $this->assertTrue(empty($sharedFolders)); + + self::loginHelper(self::TEST_FILES_SHARING_API_USER1); + + // the folder for the owner should still exists + $this->assertTrue(\OC\Files\Filesystem::file_exists($this->folder)); } } diff --git a/apps/files_sharing/tests/updater.php b/apps/files_sharing/tests/updater.php index 5ec5348870..1f51b9a315 100644 --- a/apps/files_sharing/tests/updater.php +++ b/apps/files_sharing/tests/updater.php @@ -57,7 +57,7 @@ class Test_Files_Sharing_Updater extends Test_Files_Sharing_Base { /** * test deletion of a folder which contains share mount points. Share mount - * points should move up to the parent before the folder gets deleted so + * points should be unshared before the folder gets deleted so * that the mount point doesn't end up at the trash bin */ function testDeleteParentFolder() { @@ -78,6 +78,9 @@ class Test_Files_Sharing_Updater extends Test_Files_Sharing_Base { // check if user2 can see the shared folder $this->assertTrue($view->file_exists($this->folder)); + $foldersShared = \OCP\Share::getItemsSharedWith('folder'); + $this->assertSame(1, count($foldersShared)); + $view->mkdir("localFolder"); $view->file_put_contents("localFolder/localFile.txt", "local file"); @@ -91,8 +94,9 @@ class Test_Files_Sharing_Updater extends Test_Files_Sharing_Base { $this->loginHelper(self::TEST_FILES_SHARING_API_USER2); - // mount point should move up again - $this->assertTrue($view->file_exists($this->folder)); + // shared folder should be unshared + $foldersShared = \OCP\Share::getItemsSharedWith('folder'); + $this->assertTrue(empty($foldersShared)); // trashbin should contain the local file but not the mount point $rootView = new \OC\Files\View('/' . self::TEST_FILES_SHARING_API_USER2); @@ -109,10 +113,6 @@ class Test_Files_Sharing_Updater extends Test_Files_Sharing_Base { if ($status === false) { \OC_App::disable('files_trashbin'); } - // cleanup - $this->loginHelper(self::TEST_FILES_SHARING_API_USER1); - $result = \OCP\Share::unshare('folder', $fileinfo->getId(), \OCP\Share::SHARE_TYPE_USER, self::TEST_FILES_SHARING_API_USER2); - $this->assertTrue($result); } /** From e9f573086915f6d0b86e5c57e1a2b627589f1fb6 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Wed, 2 Jul 2014 17:53:54 +0200 Subject: [PATCH 2/3] make sure that the umount hook always contains the path relative to data/user/files --- apps/files_encryption/hooks/hooks.php | 2 +- lib/private/files/view.php | 16 +++++++++++++--- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/apps/files_encryption/hooks/hooks.php b/apps/files_encryption/hooks/hooks.php index 3625d5a09f..7303cd080e 100644 --- a/apps/files_encryption/hooks/hooks.php +++ b/apps/files_encryption/hooks/hooks.php @@ -441,7 +441,7 @@ class Hooks { $ownerOld = self::$renamedFiles[$params['oldpath']]['uid']; $pathOld = self::$renamedFiles[$params['oldpath']]['path']; } else { - \OCP\Util::writeLog('Encryption library', "can't get path and owner from the file before it was renamed", \OCP\Util::ERROR); + \OCP\Util::writeLog('Encryption library', "can't get path and owner from the file before it was renamed", \OCP\Util::DEBUG); return false; } diff --git a/lib/private/files/view.php b/lib/private/files/view.php index 1a9b0e8d2a..2af693d5a6 100644 --- a/lib/private/files/view.php +++ b/lib/private/files/view.php @@ -161,17 +161,27 @@ class View { return $this->basicOperation('mkdir', $path, array('create', 'write')); } + /** + * remove mount point + * + * @param \OC\Files\Mount\MoveableMount $mount + * @param string $path relative to data/ + * @return boolean + */ protected function removeMount($mount, $path){ if ($mount instanceof MoveableMount) { + // cut of /user/files to get the relative path to data/user/files + $pathParts= explode('/', $path, 4); + $relPath = '/' . $pathParts[3]; \OC_Hook::emit( Filesystem::CLASSNAME, "umount", - array(Filesystem::signal_param_path => $path) + array(Filesystem::signal_param_path => $relPath) ); $result = $mount->removeMount(); if ($result) { \OC_Hook::emit( Filesystem::CLASSNAME, "post_umount", - array(Filesystem::signal_param_path => $path) + array(Filesystem::signal_param_path => $relPath) ); } return $result; @@ -387,7 +397,7 @@ class View { $absolutePath = Filesystem::normalizePath($this->getAbsolutePath($path)); $mount = Filesystem::getMountManager()->find($absolutePath . $postFix); if ($mount->getInternalPath($absolutePath) === '') { - return $this->removeMount($mount, $path); + return $this->removeMount($mount, $absolutePath); } return $this->basicOperation('unlink', $path, array('delete')); } From 673b0f5eb93492067a1e8560792d7a3509baa309 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Fri, 4 Jul 2014 12:06:23 +0200 Subject: [PATCH 3/3] add owner as parameter for delShareKey --- apps/files_encryption/hooks/hooks.php | 12 +++++++++--- apps/files_encryption/lib/keymanager.php | 13 +++++++------ apps/files_encryption/tests/keymanager.php | 4 ++-- 3 files changed, 18 insertions(+), 11 deletions(-) diff --git a/apps/files_encryption/hooks/hooks.php b/apps/files_encryption/hooks/hooks.php index 7303cd080e..d4a98410a3 100644 --- a/apps/files_encryption/hooks/hooks.php +++ b/apps/files_encryption/hooks/hooks.php @@ -340,7 +340,7 @@ class Hooks { } /** - * @brief + * unshare file/folder from a user with whom you shared the file before */ public static function postUnshare($params) { @@ -385,8 +385,10 @@ class Hooks { // Unshare every user who no longer has access to the file $delUsers = array_diff($userIds, $sharingUsers); + list($owner, $ownerPath) = $util->getUidAndFilename($path); + // delete share key - Keymanager::delShareKey($view, $delUsers, $path); + Keymanager::delShareKey($view, $delUsers, $ownerPath, $owner); } } @@ -595,6 +597,7 @@ class Hooks { } /** + * unmount file from yourself * remember files/folders which get unmounted */ public static function preUmount($params) { @@ -613,6 +616,9 @@ class Hooks { 'itemType' => $itemType); } + /** + * unmount file from yourself + */ public static function postUmount($params) { if (!isset(self::$umountedFiles[$params[\OC\Files\Filesystem::signal_param_path]])) { @@ -642,7 +648,7 @@ class Hooks { // check if the user still has access to the file, otherwise delete share key $sharingUsers = \OCP\Share::getUsersSharingFile($path, $user); if (!in_array(\OCP\User::getUser(), $sharingUsers['users'])) { - Keymanager::delShareKey($view, array(\OCP\User::getUser()), $path); + Keymanager::delShareKey($view, array(\OCP\User::getUser()), $path, $user); } } } diff --git a/apps/files_encryption/lib/keymanager.php b/apps/files_encryption/lib/keymanager.php index 70820a6f94..da84e975a0 100755 --- a/apps/files_encryption/lib/keymanager.php +++ b/apps/files_encryption/lib/keymanager.php @@ -444,17 +444,18 @@ class Keymanager { /** * Delete a single user's shareKey for a single file + * + * @param \OC\Files\View $view relative to data/ + * @param array $userIds list of users we want to remove + * @param string $filename the owners name of the file for which we want to remove the users relative to data/user/files + * @param string $owner owner of the file */ - public static function delShareKey(\OC\Files\View $view, $userIds, $filePath) { + public static function delShareKey($view, $userIds, $filename, $owner) { $proxyStatus = \OC_FileProxy::$enabled; \OC_FileProxy::$enabled = false; - $userId = Helper::getUser($filePath); - - $util = new Util($view, $userId); - - list($owner, $filename) = $util->getUidAndFilename($filePath); + $util = new Util($view, $owner); if ($util->isSystemWideMountPoint($filename)) { $shareKeyPath = \OC\Files\Filesystem::normalizePath('/files_encryption/share-keys/' . $filename); diff --git a/apps/files_encryption/tests/keymanager.php b/apps/files_encryption/tests/keymanager.php index eb7583650a..e779f8341e 100644 --- a/apps/files_encryption/tests/keymanager.php +++ b/apps/files_encryption/tests/keymanager.php @@ -225,7 +225,7 @@ class Test_Encryption_Keymanager extends \PHPUnit_Framework_TestCase { $this->view->file_put_contents('/'.Test_Encryption_Keymanager::TEST_USER.'/files_encryption/share-keys/folder1/subfolder/subsubfolder/file2.user3.shareKey', 'data'); // recursive delete share keys from user1 and user2 - Encryption\Keymanager::delShareKey($this->view, array('user1', 'user2', Test_Encryption_Keymanager::TEST_USER), '/folder1/'); + Encryption\Keymanager::delShareKey($this->view, array('user1', 'user2', Test_Encryption_Keymanager::TEST_USER), '/folder1/', Test_Encryption_Keymanager::TEST_USER); // check if share keys from user1 and user2 are deleted $this->assertFalse($this->view->file_exists( @@ -274,7 +274,7 @@ class Test_Encryption_Keymanager extends \PHPUnit_Framework_TestCase { $this->view->file_put_contents('/'.Test_Encryption_Keymanager::TEST_USER.'/files_encryption/share-keys/folder1/existingFile.txt.' . Test_Encryption_Keymanager::TEST_USER . '.shareKey', 'data'); // recursive delete share keys from user1 and user2 - Encryption\Keymanager::delShareKey($this->view, array('user1', 'user2', Test_Encryption_Keymanager::TEST_USER), '/folder1/existingFile.txt'); + Encryption\Keymanager::delShareKey($this->view, array('user1', 'user2', Test_Encryption_Keymanager::TEST_USER), '/folder1/existingFile.txt', Test_Encryption_Keymanager::TEST_USER); // check if share keys from user1 and user2 are deleted $this->assertFalse($this->view->file_exists(