Merge pull request #14580 from owncloud/issue/13765-duplicate-remote-share

"Integrity constraint violation" when sharing the same item twice with the same user

4 files changed, 209 insertions, 26 deletions

diff --git a/apps/files_sharing/ajax/external.php b/apps/files_sharing/ajax/external.php
index 30c1f38801e..153285e11ff 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 f2f7561598f..89a0262481c 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 490e5e5003d..5234684a81c 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';
@@ -29,7 +30,7 @@ class Manager {
 	private $mountManager;
 
 	/**
-	 * @var \OC\Files\Storage\StorageFactory
+	 * @var \OCP\Files\Storage\IStorageFactory
 	 */
 	private $storageLoader;
 
@@ -41,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;
@@ -65,33 +66,64 @@ 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) {
 
 		$user = $user ? $user : $this->uid;
 		$accepted = $accepted ? 1 : 0;
+		$name = Filesystem::normalizePath('/' . $name);
+
+		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 = '{{TemporaryMountPointName#' . $name . '}}';
+			$mountPoint = $tmpMountPointName;
+			$hash = md5($tmpMountPointName);
+			$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 (!$this->connection->insertIfNotExist('*PREFIX*share_external', $data, ['user', 'mountpoint_hash'])) {
+				// The external share already exists for the user
+				$data['mountpoint'] = $tmpMountPointName . '-' . $i;
+				$data['mountpoint_hash'] = md5($data['mountpoint']);
+				$i++;
+			}
+			return null;
+		}
 
-		$mountPoint = Filesystem::normalizePath('/' . $name);
+		$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`)
 				VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
 			');
-		$hash = md5($mountPoint);
 		$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() {
@@ -124,7 +156,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 +174,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');
 		}
 	}
@@ -315,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 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';
 
-		return $result ? $openShares->fetchAll() : array();
+		$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 00000000000..0a8c3e5f6b4
--- /dev/null
+++ b/apps/files_sharing/tests/external/managertest.php
@@ -0,0 +1,130 @@
+<?php
+/**
+ * ownCloud
+ *
+ * @author Joas Schilling
+ * @copyright 2015 Joas Schilling <nickvergessen@owncloud.com>
+ *
+ * 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 <http://www.gnu.org/licenses/>.
+ *
+ */
+
+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);
+
+	}
+}