From 05c4848954e3b2fed0f02f043dbd67777f4f6c7d Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 27 Feb 2015 16:04:17 +0100 Subject: [PATCH 1/3] Correctly get the unique mountpoint name when mounting the share Previously the mount name was checked for uniqueness prior to inserting the share. This caused problems, when two shares with the same name where done or folder, mount point, local share with the same name was done, between sending and accepting of the remote share --- apps/files_sharing/ajax/external.php | 2 - apps/files_sharing/api/server2server.php | 2 - apps/files_sharing/lib/external/manager.php | 43 ++++++++++++++++++--- 3 files changed, 37 insertions(+), 10 deletions(-) diff --git a/apps/files_sharing/ajax/external.php b/apps/files_sharing/ajax/external.php index 30c1f38801..153285e11f 100644 --- a/apps/files_sharing/ajax/external.php +++ b/apps/files_sharing/ajax/external.php @@ -38,8 +38,6 @@ $externalManager = new \OCA\Files_Sharing\External\Manager( \OC::$server->getUserSession()->getUser()->getUID() ); -$name = OCP\Files::buildNotExistingFileName('/', $name); - // check for ssl cert if (substr($remote, 0, 5) === 'https' and !OC_Util::getUrlContent($remote)) { \OCP\JSON::error(array('data' => array('message' => $l->t('Invalid or untrusted SSL certificate')))); diff --git a/apps/files_sharing/api/server2server.php b/apps/files_sharing/api/server2server.php index f2f7561598..89a0262481 100644 --- a/apps/files_sharing/api/server2server.php +++ b/apps/files_sharing/api/server2server.php @@ -64,8 +64,6 @@ class Server2Server { $shareWith ); - $name = \OCP\Files::buildNotExistingFileName('/', $name); - try { $externalManager->addShare($remote, $token, '', $name, $owner, false, $shareWith, $remoteId); diff --git a/apps/files_sharing/lib/external/manager.php b/apps/files_sharing/lib/external/manager.php index 8985aeb3fc..eb03065689 100644 --- a/apps/files_sharing/lib/external/manager.php +++ b/apps/files_sharing/lib/external/manager.php @@ -9,6 +9,7 @@ namespace OCA\Files_Sharing\External; use OC\Files\Filesystem; +use OCP\Files; class Manager { const STORAGE = '\OCA\Files_Sharing\External\Storage'; @@ -71,15 +72,39 @@ class Manager { $user = $user ? $user : $this->uid; $accepted = $accepted ? 1 : 0; + $name = Filesystem::normalizePath('/' . $name); - $mountPoint = Filesystem::normalizePath('/' . $name); + if ($accepted) { + $mountPoint = Files::buildNotExistingFileName('/', $name); + $mountPoint = Filesystem::normalizePath('/' . $mountPoint); + $hash = md5($mountPoint); + } else { + // To avoid conflicts with the mount point generation later, + // we only use a temporary mount point name here. The real + // mount point name will be generated when accepting the share, + // using the original share item name. + $tmpMountPointName = Filesystem::normalizePath('/TemporaryMountPointName-' . $name); + $mountPoint = $tmpMountPointName; + $hash = md5($tmpMountPointName); + + $query = $this->connection->prepare('SELECT `id` FROM `*PREFIX*share_external` WHERE `user` = ? AND `mountpoint_hash` = ?', 1); + $query->execute([$user, $hash]); + + $i = 1; + while ($query->fetch()) { + // The external share already exists for the user + $mountPoint = $tmpMountPointName . '-' . $i; + $hash = md5($mountPoint); + $query->execute([$user, $hash]); + $i++; + } + } $query = $this->connection->prepare(' INSERT INTO `*PREFIX*share_external` (`remote`, `share_token`, `password`, `name`, `owner`, `user`, `mountpoint`, `mountpoint_hash`, `accepted`, `remote_id`) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) '); - $hash = md5($mountPoint); $query->execute(array($remote, $token, $password, $name, $owner, $user, $mountPoint, $hash, $accepted, $remoteId)); if ($accepted) { @@ -124,7 +149,7 @@ class Manager { */ private function getShare($id) { $getShare = $this->connection->prepare(' - SELECT `remote`, `share_token` + SELECT `remote`, `share_token`, `name` FROM `*PREFIX*share_external` WHERE `id` = ? AND `user` = ?'); $result = $getShare->execute(array($id, $this->uid)); @@ -142,11 +167,17 @@ class Manager { $share = $this->getShare($id); if ($share) { + $mountPoint = Files::buildNotExistingFileName('/', $share['name']); + $mountPoint = Filesystem::normalizePath('/' . $mountPoint); + $hash = md5($mountPoint); + $acceptShare = $this->connection->prepare(' UPDATE `*PREFIX*share_external` - SET `accepted` = ? + SET `accepted` = ?, + `mountpoint` = ?, + `mountpoint_hash` = ? WHERE `id` = ? AND `user` = ?'); - $acceptShare->execute(array(1, $id, $this->uid)); + $acceptShare->execute(array(1, $mountPoint, $hash, $id, $this->uid)); $this->sendFeedbackToRemote($share['remote'], $share['share_token'], $id, 'accept'); } } @@ -321,4 +352,4 @@ class Manager { return $result ? $openShares->fetchAll() : array(); } -} \ No newline at end of file +} From ba3e4ede3901a619307f07e2aa4d4d784ff07a48 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 17 Mar 2015 10:32:11 +0100 Subject: [PATCH 2/3] Use insertIfNotExists() instead of manual logic --- apps/files_sharing/lib/external/manager.php | 55 ++++++++++++--------- 1 file changed, 31 insertions(+), 24 deletions(-) diff --git a/apps/files_sharing/lib/external/manager.php b/apps/files_sharing/lib/external/manager.php index eb03065689..592cfc4f9c 100644 --- a/apps/files_sharing/lib/external/manager.php +++ b/apps/files_sharing/lib/external/manager.php @@ -66,7 +66,7 @@ class Manager { * @param boolean $accepted * @param string $user * @param int $remoteId - * @return mixed + * @return Mount|null */ public function addShare($remote, $token, $password, $name, $owner, $accepted=false, $user = null, $remoteId = -1) { @@ -74,32 +74,41 @@ class Manager { $accepted = $accepted ? 1 : 0; $name = Filesystem::normalizePath('/' . $name); - if ($accepted) { - $mountPoint = Files::buildNotExistingFileName('/', $name); - $mountPoint = Filesystem::normalizePath('/' . $mountPoint); - $hash = md5($mountPoint); - } else { + if (!$accepted) { // To avoid conflicts with the mount point generation later, // we only use a temporary mount point name here. The real // mount point name will be generated when accepting the share, // using the original share item name. - $tmpMountPointName = Filesystem::normalizePath('/TemporaryMountPointName-' . $name); + $tmpMountPointName = '{{TemporaryMountPointName#' . $name . '}}'; $mountPoint = $tmpMountPointName; $hash = md5($tmpMountPointName); - - $query = $this->connection->prepare('SELECT `id` FROM `*PREFIX*share_external` WHERE `user` = ? AND `mountpoint_hash` = ?', 1); - $query->execute([$user, $hash]); + $data = [ + 'remote' => $remote, + 'share_token' => $token, + 'password' => $password, + 'name' => $name, + 'owner' => $owner, + 'user' => $user, + 'mountpoint' => $mountPoint, + 'mountpoint_hash' => $hash, + 'accepted' => $accepted, + 'remote_id' => $remoteId, + ]; $i = 1; - while ($query->fetch()) { + while (!$this->connection->insertIfNotExist('*PREFIX*share_external', $data, ['user', 'mountpoint_hash'])) { // The external share already exists for the user - $mountPoint = $tmpMountPointName . '-' . $i; - $hash = md5($mountPoint); - $query->execute([$user, $hash]); + $data['mountpoint'] = $tmpMountPointName . '-' . $i; + $data['mountpoint_hash'] = md5($data['mountpoint']); $i++; } + return null; } + $mountPoint = Files::buildNotExistingFileName('/', $name); + $mountPoint = Filesystem::normalizePath('/' . $mountPoint); + $hash = md5($mountPoint); + $query = $this->connection->prepare(' INSERT INTO `*PREFIX*share_external` (`remote`, `share_token`, `password`, `name`, `owner`, `user`, `mountpoint`, `mountpoint_hash`, `accepted`, `remote_id`) @@ -107,16 +116,14 @@ class Manager { '); $query->execute(array($remote, $token, $password, $name, $owner, $user, $mountPoint, $hash, $accepted, $remoteId)); - if ($accepted) { - $options = array( - 'remote' => $remote, - 'token' => $token, - 'password' => $password, - 'mountpoint' => $mountPoint, - 'owner' => $owner - ); - return $this->mountShare($options); - } + $options = array( + 'remote' => $remote, + 'token' => $token, + 'password' => $password, + 'mountpoint' => $mountPoint, + 'owner' => $owner + ); + return $this->mountShare($options); } private function setupMounts() { From 8ebb198ef38670859be8d42c13ddebd36a894381 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 17 Mar 2015 16:51:07 +0100 Subject: [PATCH 3/3] Add a unit test for the naming conflict on the mountpoint name --- apps/files_sharing/lib/external/manager.php | 31 ++++- .../tests/external/managertest.php | 130 ++++++++++++++++++ 2 files changed, 155 insertions(+), 6 deletions(-) create mode 100644 apps/files_sharing/tests/external/managertest.php diff --git a/apps/files_sharing/lib/external/manager.php b/apps/files_sharing/lib/external/manager.php index 592cfc4f9c..5234684a81 100644 --- a/apps/files_sharing/lib/external/manager.php +++ b/apps/files_sharing/lib/external/manager.php @@ -30,7 +30,7 @@ class Manager { private $mountManager; /** - * @var \OC\Files\Storage\StorageFactory + * @var \OCP\Files\Storage\IStorageFactory */ private $storageLoader; @@ -42,12 +42,12 @@ class Manager { /** * @param \OCP\IDBConnection $connection * @param \OC\Files\Mount\Manager $mountManager - * @param \OC\Files\Storage\StorageFactory $storageLoader + * @param \OCP\Files\Storage\IStorageFactory $storageLoader * @param \OC\HTTPHelper $httpHelper * @param string $uid */ public function __construct(\OCP\IDBConnection $connection, \OC\Files\Mount\Manager $mountManager, - \OC\Files\Storage\StorageFactory $storageLoader, \OC\HTTPHelper $httpHelper, $uid) { + \OCP\Files\Storage\IStorageFactory $storageLoader, \OC\HTTPHelper $httpHelper, $uid) { $this->connection = $connection; $this->mountManager = $mountManager; $this->storageLoader = $storageLoader; @@ -353,10 +353,29 @@ class Manager { * @return array list of open server-to-server shares */ public function getOpenShares() { - $openShares = $this->connection->prepare('SELECT * FROM `*PREFIX*share_external` WHERE `accepted` = ? AND `user` = ?'); - $result = $openShares->execute(array(0, $this->uid)); + return $this->getShares(false); + } - return $result ? $openShares->fetchAll() : array(); + /** + * return a list of shares for the user + * + * @param bool|null $accepted True for accepted only, + * false for not accepted, + * null for all shares of the user + * @return array list of open server-to-server shares + */ + private function getShares($accepted) { + $query = 'SELECT * FROM `*PREFIX*share_external` WHERE `user` = ?'; + $parameters = [$this->uid]; + if (!is_null($accepted)) { + $query .= 'AND `accepted` = ?'; + $parameters[] = (int) $accepted; + } + $query .= ' ORDER BY `id` ASC'; + $shares = $this->connection->prepare($query); + $result = $shares->execute($parameters); + + return $result ? $shares->fetchAll() : []; } } diff --git a/apps/files_sharing/tests/external/managertest.php b/apps/files_sharing/tests/external/managertest.php new file mode 100644 index 0000000000..0a8c3e5f6b --- /dev/null +++ b/apps/files_sharing/tests/external/managertest.php @@ -0,0 +1,130 @@ + + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU AFFERO GENERAL PUBLIC LICENSE + * License as published by the Free Software Foundation; either + * version 3 of the License, or any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU AFFERO GENERAL PUBLIC LICENSE for more details. + * + * You should have received a copy of the GNU Affero General Public + * License along with this library. If not, see . + * + */ + +namespace OCA\Files_Sharing\Tests\External; + +use OCA\Files_Sharing\Tests\TestCase; + +class ManagerTest extends TestCase { + + /** @var \OCA\Files_Sharing\External\Manager **/ + private $manager; + + private $uid; + + protected function setUp() { + parent::setUp(); + + $this->uid = $this->getUniqueID('user'); + $this->manager = new \OCA\Files_Sharing\External\Manager( + \OC::$server->getDatabaseConnection(), + $this->getMockBuilder('\OC\Files\Mount\Manager')->disableOriginalConstructor()->getMock(), + $this->getMockBuilder('\OCP\Files\Storage\IStorageFactory')->disableOriginalConstructor()->getMock(), + $this->getMockBuilder('\OC\HTTPHelper')->disableOriginalConstructor()->getMock(), + $this->uid + ); + } + + public function testAddShare() { + $shareData1 = [ + 'remote' => 'localhost', + 'token' => 'token1', + 'password' => '', + 'name' => '/SharedFolder', + 'owner' => 'foobar', + 'accepted' => false, + 'user' => $this->uid, + ]; + $shareData2 = $shareData1; + $shareData2['token'] = 'token2'; + $shareData3 = $shareData1; + $shareData3['token'] = 'token3'; + + // Add a share for "user" + $this->assertSame(null, call_user_func_array([$this->manager, 'addShare'], $shareData1)); + $openShares = $this->manager->getOpenShares(); + $this->assertCount(1, $openShares); + $this->assertExternalShareEntry($shareData1, $openShares[0], 1, '{{TemporaryMountPointName#' . $shareData1['name'] . '}}'); + + // Add a second share for "user" with the same name + $this->assertSame(null, call_user_func_array([$this->manager, 'addShare'], $shareData2)); + $openShares = $this->manager->getOpenShares(); + $this->assertCount(2, $openShares); + $this->assertExternalShareEntry($shareData1, $openShares[0], 1, '{{TemporaryMountPointName#' . $shareData1['name'] . '}}'); + // New share falls back to "-1" appendix, because the name is already taken + $this->assertExternalShareEntry($shareData2, $openShares[1], 2, '{{TemporaryMountPointName#' . $shareData2['name'] . '}}-1'); + + // Accept the first share + $this->manager->acceptShare($openShares[0]['id']); + + // Check remaining shares - Accepted + $acceptedShares = \Test_Helper::invokePrivate($this->manager, 'getShares', [true]); + $this->assertCount(1, $acceptedShares); + $shareData1['accepted'] = true; + $this->assertExternalShareEntry($shareData1, $acceptedShares[0], 1, $shareData1['name']); + // Check remaining shares - Open + $openShares = $this->manager->getOpenShares(); + $this->assertCount(1, $openShares); + $this->assertExternalShareEntry($shareData2, $openShares[0], 2, '{{TemporaryMountPointName#' . $shareData2['name'] . '}}-1'); + + // Add another share for "user" with the same name + $this->assertSame(null, call_user_func_array([$this->manager, 'addShare'], $shareData3)); + $openShares = $this->manager->getOpenShares(); + $this->assertCount(2, $openShares); + $this->assertExternalShareEntry($shareData2, $openShares[0], 2, '{{TemporaryMountPointName#' . $shareData2['name'] . '}}-1'); + // New share falls back to the original name (no "-\d", because the name is not taken) + $this->assertExternalShareEntry($shareData3, $openShares[1], 3, '{{TemporaryMountPointName#' . $shareData3['name'] . '}}'); + + // Decline the third share + $this->manager->declineShare($openShares[1]['id']); + + // Check remaining shares - Accepted + $acceptedShares = \Test_Helper::invokePrivate($this->manager, 'getShares', [true]); + $this->assertCount(1, $acceptedShares); + $shareData1['accepted'] = true; + $this->assertExternalShareEntry($shareData1, $acceptedShares[0], 1, $shareData1['name']); + // Check remaining shares - Open + $openShares = $this->manager->getOpenShares(); + $this->assertCount(1, $openShares); + $this->assertExternalShareEntry($shareData2, $openShares[0], 2, '{{TemporaryMountPointName#' . $shareData2['name'] . '}}-1'); + + $this->manager->removeUserShares($this->uid); + $this->assertEmpty(\Test_Helper::invokePrivate($this->manager, 'getShares', [null]), 'Asserting all shares for the user have been deleted'); + } + + /** + * @param array $expected + * @param array $actual + * @param int $share + * @param string $mountPoint + */ + protected function assertExternalShareEntry($expected, $actual, $share, $mountPoint) { + $this->assertEquals($expected['remote'], $actual['remote'], 'Asserting remote of a share #' . $share); + $this->assertEquals($expected['token'], $actual['share_token'], 'Asserting token of a share #' . $share); + $this->assertEquals($expected['name'], $actual['name'], 'Asserting name of a share #' . $share); + $this->assertEquals($expected['owner'], $actual['owner'], 'Asserting owner of a share #' . $share); + $this->assertEquals($expected['accepted'], (int) $actual['accepted'], 'Asserting accept of a share #' . $share); + $this->assertEquals($expected['user'], $actual['user'], 'Asserting user of a share #' . $share); + $this->assertEquals($mountPoint, $actual['mountpoint'], 'Asserting mountpoint of a share #' . $share); + + } +}