From 80f83ab5e079489a46ed56adee1898afb8b26ff0 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Tue, 24 Mar 2015 11:08:19 +0100 Subject: [PATCH 1/2] Do not return shares for files outside "files" --- lib/private/share/share.php | 59 ++++++++++++++++++++++++++++++------- tests/lib/share/share.php | 39 ++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 11 deletions(-) diff --git a/lib/private/share/share.php b/lib/private/share/share.php index aeb9a9a476..9500ec6942 100644 --- a/lib/private/share/share.php +++ b/lib/private/share/share.php @@ -327,18 +327,20 @@ class Share extends \OC\Share\Constants { */ public static function getItemSharedWithUser($itemType, $itemSource, $user, $owner = null, $shareType = null) { $shares = array(); - $fileDependend = false; + $fileDependent = false; if ($itemType === 'file' || $itemType === 'folder') { - $fileDependend = true; + $fileDependent = true; $column = 'file_source'; - $where = 'INNER JOIN `*PREFIX*filecache` ON `file_source` = `*PREFIX*filecache`.`fileid` WHERE'; + $where = 'INNER JOIN `*PREFIX*filecache` ON `file_source` = `*PREFIX*filecache`.`fileid` '; + $where .= 'INNER JOIN `*PREFIX*storages` ON `numeric_id` = `*PREFIX*filecache`.`storage` '; + $where .= ' WHERE'; } else { $column = 'item_source'; $where = 'WHERE'; } - $select = self::createSelectStatement(self::FORMAT_NONE, $fileDependend); + $select = self::createSelectStatement(self::FORMAT_NONE, $fileDependent); $where .= ' `' . $column . '` = ? AND `item_type` = ? '; $arguments = array($itemSource, $itemType); @@ -363,6 +365,9 @@ class Share extends \OC\Share\Constants { $result = \OC_DB::executeAudited($query, $arguments); while ($row = $result->fetchRow()) { + if ($fileDependent && !self::isFileReachable($row['path'], $row['storage_id'])) { + continue; + } $shares[] = $row; } @@ -1382,10 +1387,11 @@ class Share extends \OC\Share\Constants { } else { $root = ''; } - $where = 'INNER JOIN `*PREFIX*filecache` ON `file_source` = `*PREFIX*filecache`.`fileid`'; + $where = 'INNER JOIN `*PREFIX*filecache` ON `file_source` = `*PREFIX*filecache`.`fileid` '; if (!isset($item)) { - $where .= ' WHERE `file_target` IS NOT NULL'; + $where .= ' AND `file_target` IS NOT NULL '; } + $where .= 'INNER JOIN `*PREFIX*storages` ON `numeric_id` = `*PREFIX*filecache`.`storage` '; $fileDependent = true; $queryArgs = array(); } else { @@ -1526,6 +1532,9 @@ class Share extends \OC\Share\Constants { while ($row = $result->fetchRow()) { self::transformDBResults($row); // Filter out duplicate group shares for users with unique targets + if ($fileDependent && !self::isFileReachable($row['path'], $row['storage_id'])) { + continue; + } if ($row['share_type'] == self::$shareTypeGroupUserUnique && isset($items[$row['parent']])) { $row['share_type'] = self::SHARE_TYPE_GROUP; $row['unique_name'] = true; // remember that we use a unique name for this user @@ -2181,7 +2190,9 @@ class Share extends \OC\Share\Constants { $select = '*'; if ($format == self::FORMAT_STATUSES) { if ($fileDependent) { - $select = '`*PREFIX*share`.`id`, `*PREFIX*share`.`parent`, `share_type`, `path`, `storage`, `share_with`, `uid_owner` , `file_source`, `stime`, `*PREFIX*share`.`permissions`'; + $select = '`*PREFIX*share`.`id`, `*PREFIX*share`.`parent`, `share_type`, `path`, `storage`, ' + . '`share_with`, `uid_owner` , `file_source`, `stime`, `*PREFIX*share`.`permissions`, ' + . '`*PREFIX*storages`.`id` AS `storage_id`'; } else { $select = '`id`, `parent`, `share_type`, `share_with`, `uid_owner`, `item_source`, `stime`, `*PREFIX*share`.`permissions`'; } @@ -2190,7 +2201,8 @@ class Share extends \OC\Share\Constants { if ($fileDependent) { $select = '`*PREFIX*share`.`id`, `item_type`, `item_source`, `*PREFIX*share`.`parent`,' . ' `share_type`, `share_with`, `file_source`, `file_target`, `path`, `*PREFIX*share`.`permissions`, `stime`,' - . ' `expiration`, `token`, `storage`, `mail_send`, `uid_owner`'; + . ' `expiration`, `token`, `storage`, `mail_send`, `uid_owner`, ' + . '`*PREFIX*storages`.`id` AS `storage_id`'; } else { $select = '`id`, `item_type`, `item_source`, `parent`, `share_type`, `share_with`, `*PREFIX*share`.`permissions`,' . ' `stime`, `file_source`, `expiration`, `token`, `mail_send`, `uid_owner`'; @@ -2203,9 +2215,11 @@ class Share extends \OC\Share\Constants { . '`*PREFIX*share`.`permissions`, `expiration`, `storage`, `*PREFIX*filecache`.`parent` as `file_parent`, ' . '`name`, `mtime`, `mimetype`, `mimepart`, `size`, `unencrypted_size`, `encrypted`, `etag`, `mail_send`'; } else { - $select = '`*PREFIX*share`.`id`, `item_type`, `item_source`, `item_target`, - `*PREFIX*share`.`parent`, `share_type`, `share_with`, `uid_owner`, - `file_source`, `path`, `file_target`, `*PREFIX*share`.`permissions`, `stime`, `expiration`, `token`, `storage`, `mail_send`'; + $select = '`*PREFIX*share`.`id`, `item_type`, `item_source`, `item_target`,' + . '`*PREFIX*share`.`parent`, `share_type`, `share_with`, `uid_owner`,' + . '`file_source`, `path`, `file_target`, `*PREFIX*share`.`permissions`,' + . '`stime`, `expiration`, `token`, `storage`, `mail_send`,' + . '`*PREFIX*storages`.`id` AS `storage_id`'; } } } @@ -2391,4 +2405,27 @@ class Share extends \OC\Share\Constants { return (int)\OCP\Config::getAppValue('core', 'shareapi_expire_after_n_days', '7'); } + /** + * Checks whether the given path is reachable for the given owner + * + * @param string $path path relative to files + * @param string $ownerStorageId storage id of the owner + * + * @return boolean true if file is reachable, false otherwise + */ + private static function isFileReachable($path, $ownerStorageId) { + // if outside the home storage, file is always considered reachable + if (!(substr($ownerStorageId, 0, 6) === 'home::')) { + return true; + } + + // if inside the home storage, the file has to be under "/files/" + $path = ltrim($path, '/'); + if (substr($path, 0, 6) === 'files/') { + return true; + } + + return false; + } + } diff --git a/tests/lib/share/share.php b/tests/lib/share/share.php index 42bb82968a..2bc1e320e5 100644 --- a/tests/lib/share/share.php +++ b/tests/lib/share/share.php @@ -388,6 +388,45 @@ class Test_Share extends \Test\TestCase { $this->assertSame(\OCP\Share::SHARE_TYPE_USER, $share['share_type']); } + public function testGetShareFromOutsideFilesFolder() { + OC_User::setUserId($this->user1); + $view = new \OC\Files\View('/' . $this->user1 . '/'); + $view->mkdir('files/test'); + $view->mkdir('files/test/sub'); + + $view->mkdir('files_trashbin'); + $view->mkdir('files_trashbin/files'); + + $fileInfo = $view->getFileInfo('files/test/sub'); + $fileId = $fileInfo->getId(); + + $this->assertTrue( + OCP\Share::shareItem('folder', $fileId, OCP\Share::SHARE_TYPE_USER, $this->user2, \OCP\Constants::PERMISSION_READ), + 'Failed asserting that user 1 successfully shared "test/sub" with user 2.' + ); + + $result = OCP\Share::getItemShared('folder', $fileId, Test_Share_Backend::FORMAT_SOURCE); + $this->assertNotEmpty($result); + + $result = OCP\Share::getItemSharedWithUser('folder', $fileId, $this->user2); + $this->assertNotEmpty($result); + + $result = OCP\Share::getItemsSharedWithUser('folder', $this->user2); + $this->assertNotEmpty($result); + + // move to trash (keeps file id) + $view->rename('files/test', 'files_trashbin/files/test'); + + $result = OCP\Share::getItemShared('folder', $fileId, Test_Share_Backend::FORMAT_SOURCE); + $this->assertEmpty($result, 'Share must not be returned for files outside of "files"'); + + $result = OCP\Share::getItemSharedWithUser('folder', $fileId, $this->user2); + $this->assertEmpty($result, 'Share must not be returned for files outside of "files"'); + + $result = OCP\Share::getItemsSharedWithUser('folder', $this->user2); + $this->assertEmpty($result, 'Share must not be returned for files outside of "files"'); + } + public function testSetExpireDateInPast() { OC_User::setUserId($this->user1); $this->shareUserOneTestFileWithUserTwo(); From 23cb8112fcea5a93dfedbe26433dd1f469c98670 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 27 Mar 2015 15:54:29 +0100 Subject: [PATCH 2/2] Add logout in share test to avoid nasty side effects --- tests/lib/share/share.php | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/lib/share/share.php b/tests/lib/share/share.php index 2bc1e320e5..4353986650 100644 --- a/tests/lib/share/share.php +++ b/tests/lib/share/share.php @@ -101,6 +101,7 @@ class Test_Share extends \Test\TestCase { OC_Group::deleteGroup($this->group2); OC_Group::deleteGroup($this->groupAndUser); + $this->logout(); parent::tearDown(); }