From 763b601e4aa109bcade4af687c4c6a9b8e66b5df Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Fri, 3 Jul 2015 15:58:52 +0200 Subject: [PATCH 1/2] use the correct user if we update the share table --- apps/files_sharing/lib/sharedmount.php | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/apps/files_sharing/lib/sharedmount.php b/apps/files_sharing/lib/sharedmount.php index fd672d0500..2771e0415b 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 From 058d910f5e85ca71d20272d8ca89dc3caf64cef2 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Fri, 3 Jul 2015 17:04:05 +0200 Subject: [PATCH 2/2] intproduce pre_addToGroup hook. we need to calculate the possible unique targets before the user was added to the group otherwise we will always detect a name collision --- lib/base.php | 1 + lib/private/share/hooks.php | 123 +++++++++++++++++++++++++--------- tests/lib/share/hooktests.php | 108 +++++++++++++++++++++++++++++ 3 files changed, 200 insertions(+), 32 deletions(-) create mode 100644 tests/lib/share/hooktests.php diff --git a/lib/base.php b/lib/base.php index 8812d5698f..bb6027370b 100644 --- a/lib/base.php +++ b/lib/base.php @@ -797,6 +797,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 b0d4f0677f..98143124e8 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 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 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 + $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']); + + $result = $select->execute(); + + 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 { + $fileTarget = null; + } + + + // Insert an extra row for the group share if the item or file target is unique for this user + 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. - * TODO what does it do? + * 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(); - // 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()) { + $insert = $db->createQueryBuilder(); + $insert->insert('`*PREFIX*share`'); - $sourceExists = \OC\Share\Share::getItemSharedWithBySource($item['item_type'], $item['item_source'], self::FORMAT_NONE, null, true, $arguments['uid']); - - if ($sourceExists) { - $fileTarget = $sourceExists['file_target']; - $itemTarget = $sourceExists['item_target']; - } 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; - } - } - - // 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 (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 0000000000..f980baf357 --- /dev/null +++ b/tests/lib/share/hooktests.php @@ -0,0 +1,108 @@ + + * + * @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 + * + */ + + +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']); + } + } + +}