Merge pull request #17381 from owncloud/fix_sharing_add_to_group

[sharing] fix addToGroup hook

4 files changed, 206 insertions, 32 deletions

diff --git a/apps/files_sharing/lib/sharedmount.php b/apps/files_sharing/lib/sharedmount.php
index fd672d0500c..2771e0415b0 100644
--- a/apps/files_sharing/lib/sharedmount.php
+++ b/apps/files_sharing/lib/sharedmount.php
@@ -46,12 +46,18 @@ class SharedMount extends MountPoint implements MoveableMount {
 	 */
 	private $recipientView;
 
+	/**
+	 * @var string
+	 */
+	private $user;
+
 	public function __construct($storage, $mountpoint, $arguments = null, $loader = null) {
 		// first update the mount point before creating the parent
 		$this->ownerPropagator = $arguments['propagator'];
-		$this->recipientView = new View('/' . $arguments['user'] . '/files');
+		$this->user = $arguments['user'];
+		$this->recipientView = new View('/' . $this->user . '/files');
 		$newMountPoint = $this->verifyMountPoint($arguments['share']);
-		$absMountPoint = '/' . $arguments['user'] . '/files' . $newMountPoint;
+		$absMountPoint = '/' . $this->user . '/files' . $newMountPoint;
 		$arguments['ownerView'] = new View('/' . $arguments['share']['uid_owner'] . '/files');
 		parent::__construct($storage, $absMountPoint, $arguments, $loader);
 	}
@@ -90,7 +96,7 @@ class SharedMount extends MountPoint implements MoveableMount {
 	 * @param array $share reference to the share which should be modified
 	 * @return bool
 	 */
-	private static function updateFileTarget($newPath, &$share) {
+	private function updateFileTarget($newPath, &$share) {
 		// if the user renames a mount point from a group share we need to create a new db entry
 		// for the unique name
 		if ($share['share_type'] === \OCP\Share::SHARE_TYPE_GROUP && empty($share['unique_name'])) {
@@ -98,7 +104,7 @@ class SharedMount extends MountPoint implements MoveableMount {
 			.' `share_type`, `share_with`, `uid_owner`, `permissions`, `stime`, `file_source`,'
 			.' `file_target`, `token`, `parent`) VALUES (?,?,?,?,?,?,?,?,?,?,?,?)');
 			$arguments = array($share['item_type'], $share['item_source'], $share['item_target'],
-				2, \OCP\User::getUser(), $share['uid_owner'], $share['permissions'], $share['stime'], $share['file_source'],
+				2, $this->user, $share['uid_owner'], $share['permissions'], $share['stime'], $share['file_source'],
 				$newPath, $share['token'], $share['id']);
 		} else {
 			// rename mount point
diff --git a/lib/base.php b/lib/base.php
index 9a682a7e90f..32fcbac7c05 100644
--- a/lib/base.php
+++ b/lib/base.php
@@ -800,6 +800,7 @@ class OC {
 		if (\OC::$server->getSystemConfig()->getValue('installed')) {
 			OC_Hook::connect('OC_User', 'post_deleteUser', 'OC\Share\Hooks', 'post_deleteUser');
 			OC_Hook::connect('OC_User', 'post_addToGroup', 'OC\Share\Hooks', 'post_addToGroup');
+			OC_Hook::connect('OC_Group', 'pre_addToGroup', 'OC\Share\Hooks', 'pre_addToGroup');
 			OC_Hook::connect('OC_User', 'post_removeFromGroup', 'OC\Share\Hooks', 'post_removeFromGroup');
 			OC_Hook::connect('OC_User', 'post_deleteGroup', 'OC\Share\Hooks', 'post_deleteGroup');
 		}
diff --git a/lib/private/share/hooks.php b/lib/private/share/hooks.php
index b0d4f0677f5..98143124e82 100644
--- a/lib/private/share/hooks.php
+++ b/lib/private/share/hooks.php
@@ -24,6 +24,13 @@
 namespace OC\Share;
 
 class Hooks extends \OC\Share\Constants {
+
+	/**
+	 * remember which targets need to be updated in the post addToGroup Hook
+	 * @var array
+	 */
+	private static $updateTargets = array();
+
 	/**
 	 * Function that is called after a user is deleted. Cleans up the shares of that user.
 	 * @param array $arguments
@@ -41,46 +48,98 @@ class Hooks extends \OC\Share\Constants {
 		}
 	}
 
+
 	/**
-	 * Function that is called after a user is added to a group.
-	 * TODO what does it do?
+	 * Function that is called before a user is added to a group.
+	 * check if we need to create a unique target for the user
 	 * @param array $arguments
 	 */
-	public static function post_addToGroup($arguments) {
+	public static function pre_addToGroup($arguments) {
+		/** @var \OC\DB\Connection $db */
+		$db = \OC::$server->getDatabaseConnection();
+
+		$insert = $db->createQueryBuilder();
 
+		$select = $db->createQueryBuilder();
 		// Find the group shares and check if the user needs a unique target
-		$query = \OC_DB::prepare('SELECT * FROM `*PREFIX*share` WHERE `share_type` = ? AND `share_with` = ?');
-		$result = $query->execute(array(self::SHARE_TYPE_GROUP, $arguments['gid']));
-		$query = \OC_DB::prepare('INSERT INTO `*PREFIX*share` (`item_type`, `item_source`,'
-			.' `item_target`, `parent`, `share_type`, `share_with`, `uid_owner`, `permissions`,'
-			.' `stime`, `file_source`, `file_target`) VALUES (?,?,?,?,?,?,?,?,?,?,?)');
-		while ($item = $result->fetchRow()) {
+		$select->select('*')
+			->from('`*PREFIX*share`')
+			->where($select->expr()->andX(
+				$select->expr()->eq('`share_type`', ':shareType'),
+				$select->expr()->eq('`share_with`', ':shareWith')
+			))
+			->setParameter('shareType', self::SHARE_TYPE_GROUP)
+			->setParameter('shareWith', $arguments['gid']);
 
-			$sourceExists = \OC\Share\Share::getItemSharedWithBySource($item['item_type'], $item['item_source'], self::FORMAT_NONE, null, true, $arguments['uid']);
+		$result = $select->execute();
 
-			if ($sourceExists) {
-				$fileTarget = $sourceExists['file_target'];
-				$itemTarget = $sourceExists['item_target'];
+		while ($item = $result->fetch()) {
+
+			$itemTarget = Helper::generateTarget(
+				$item['item_type'],
+				$item['item_source'],
+				self::SHARE_TYPE_USER,
+				$arguments['uid'],
+				$item['uid_owner'],
+				null,
+				$item['parent']
+			);
+
+			if ($item['item_type'] === 'file' || $item['item_type'] === 'folder') {
+				$fileTarget = Helper::generateTarget(
+					$item['item_type'],
+					$item['file_target'],
+					self::SHARE_TYPE_USER,
+					$arguments['uid'],
+					$item['uid_owner'],
+					null,
+					$item['parent']
+				);
 			} else {
-				$itemTarget = Helper::generateTarget($item['item_type'], $item['item_source'], self::SHARE_TYPE_USER, $arguments['uid'],
-					$item['uid_owner'], null, $item['parent']);
-
-				// do we also need a file target
-				if ($item['item_type'] === 'file' || $item['item_type'] === 'folder') {
-					$fileTarget = Helper::generateTarget('file', $item['file_target'], self::SHARE_TYPE_USER, $arguments['uid'],
-							$item['uid_owner'], null, $item['parent']);
-				} else {
-					$fileTarget = null;
-				}
+				$fileTarget = null;
 			}
 
+
 			// Insert an extra row for the group share if the item or file target is unique for this user
-			if ($itemTarget != $item['item_target'] || $fileTarget != $item['file_target']) {
-				$query->execute(array($item['item_type'], $item['item_source'], $itemTarget, $item['id'],
-					self::$shareTypeGroupUserUnique, $arguments['uid'], $item['uid_owner'], $item['permissions'],
-					$item['stime'], $item['file_source'], $fileTarget));
-				\OC_DB::insertid('*PREFIX*share');
+			if (
+				($fileTarget === null && $itemTarget != $item['item_target'])
+				|| ($fileTarget !== null && $fileTarget !== $item['file_target'])
+			) {
+				self::$updateTargets[$arguments['gid']][] = [
+					'`item_type`' => $insert->expr()->literal($item['item_type']),
+					'`item_source`' => $insert->expr()->literal($item['item_source']),
+					'`item_target`' => $insert->expr()->literal($itemTarget),
+					'`file_target`' => $insert->expr()->literal($fileTarget),
+					'`parent`' => $insert->expr()->literal($item['id']),
+					'`share_type`' => $insert->expr()->literal(self::$shareTypeGroupUserUnique),
+					'`share_with`' => $insert->expr()->literal($arguments['uid']),
+					'`uid_owner`' => $insert->expr()->literal($item['uid_owner']),
+					'`permissions`' => $insert->expr()->literal($item['permissions']),
+					'`stime`' => $insert->expr()->literal($item['stime']),
+					'`file_source`' => $insert->expr()->literal($item['file_source']),
+				];
+			}
+		}
+	}
+
+	/**
+	 * Function that is called after a user is added to a group.
+	 * add unique target for the user if needed
+	 * @param array $arguments
+	 */
+	public static function post_addToGroup($arguments) {
+		/** @var \OC\DB\Connection $db */
+		$db = \OC::$server->getDatabaseConnection();
+
+		$insert = $db->createQueryBuilder();
+		$insert->insert('`*PREFIX*share`');
+
+		if (isset(self::$updateTargets[$arguments['gid']])) {
+			foreach (self::$updateTargets[$arguments['gid']] as $newTarget) {
+				$insert->values($newTarget);
+				$insert->execute();
 			}
+			unset(self::$updateTargets[$arguments['gid']]);
 		}
 	}
 
diff --git a/tests/lib/share/hooktests.php b/tests/lib/share/hooktests.php
new file mode 100644
index 00000000000..f980baf3574
--- /dev/null
+++ b/tests/lib/share/hooktests.php
@@ -0,0 +1,108 @@
+<?php
+/**
+ * @author Björn Schießle <schiessle@owncloud.com>
+ *
+ * @copyright Copyright (c) 2015, ownCloud, Inc.
+ * @license AGPL-3.0
+ *
+ * This code is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License, version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program 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, version 3,
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>
+ *
+ */
+
+
+namespace OC\Tests\Share;
+
+
+use Test\TestCase;
+
+class HookTests extends TestCase {
+
+	protected function setUp() {
+		parent::setUp();
+	}
+
+	protected function tearDown() {
+		$query = \OC_DB::prepare('DELETE FROM `*PREFIX*share` WHERE `item_type` = ?');
+		$query->execute(array('test'));
+
+		parent::tearDown();
+	}
+
+	public function testPostAddToGroup() {
+
+		/** @var \OC\DB\Connection $connection */
+		$connection = \OC::$server->getDatabaseConnection();
+		$query = $connection->createQueryBuilder();
+		$expr = $query->expr();
+
+		// add some dummy values to the private $updateTargets variable
+		$this->invokePrivate(
+			new \OC\Share\Hooks(),
+			'updateTargets',
+			[
+				[
+					'group1' =>
+						[
+							[
+								'`item_type`' => $expr->literal('test'),
+								'`item_source`' => $expr->literal('42'),
+								'`item_target`' => $expr->literal('42'),
+								'`file_target`' => $expr->literal('test'),
+								'`share_type`' => $expr->literal('2'),
+								'`share_with`' => $expr->literal('group1'),
+								'`uid_owner`' => $expr->literal('owner'),
+								'`permissions`' => $expr->literal('0'),
+								'`stime`' => $expr->literal('676584'),
+								'`file_source`' => $expr->literal('42'),
+							],
+							[
+								'`item_type`' => $expr->literal('test'),
+								'`item_source`' => $expr->literal('42'),
+								'`item_target`' => $expr->literal('42 (2)'),
+								'`share_type`' => $expr->literal('2'),
+								'`share_with`' => $expr->literal('group1'),
+								'`uid_owner`' => $expr->literal('owner'),
+								'`permissions`' => $expr->literal('0'),
+								'`stime`' => $expr->literal('676584'),
+							]
+						],
+					'group2' =>
+						[
+							[
+								'`item_type`' => $expr->literal('test'),
+								'`item_source`' => $expr->literal('42'),
+								'`item_target`' => $expr->literal('42'),
+								'`share_type`' => $expr->literal('2'),
+								'`share_with`' => $expr->literal('group2'),
+								'`uid_owner`' => $expr->literal('owner'),
+								'`permissions`' => $expr->literal('0'),
+								'`stime`' => $expr->literal('676584'),
+							]
+						]
+				]
+			]
+		);
+
+		// add unique targets for group1 to database
+		\OC\Share\Hooks::post_addToGroup(['gid' => 'group1']);
+
+
+		$query->select('`share_with`')->from('`*PREFIX*share`');
+		$result = $query->execute()->fetchAll();
+		$this->assertSame(2, count($result));
+		foreach ($result as $r) {
+			$this->assertSame('group1', $r['share_with']);
+		}
+	}
+
+}