From f149bce242c2f94ced92c4895adb10f6b1f8c7b4 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Mon, 11 Jul 2016 17:33:24 +0200 Subject: [PATCH 01/12] Add integration tests for merging received shares --- build/integration/features/sharing-v1.feature | 113 ++++++++++++++++++ 1 file changed, 113 insertions(+) diff --git a/build/integration/features/sharing-v1.feature b/build/integration/features/sharing-v1.feature index da6ab1d71d36..5f5e9fb21dc5 100644 --- a/build/integration/features/sharing-v1.feature +++ b/build/integration/features/sharing-v1.feature @@ -608,3 +608,116 @@ Feature: sharing | /foo/ | | /foo%20(2)/ | + Scenario: Merging shares for recipient when shared from outside with group and member + Given As an "admin" + And user "user0" exists + And user "user1" exists + And group "group1" exists + And user "user1" belongs to group "group1" + And user "user0" created a folder "merge-test-outside" + When folder "merge-test-outside" of user "user0" is shared with group "group1" + And folder "merge-test-outside" of user "user0" is shared with user "user1" + Then as "user1" the folder "merge-test-outside" exists + And as "user1" the folder "merge-test-outside (2)" does not exist + + Scenario: Merging shares for recipient when shared from outside with group and member with different permissions + Given As an "admin" + And user "user0" exists + And user "user1" exists + And group "group1" exists + And user "user1" belongs to group "group1" + And user "user0" created a folder "merge-test-outside-perms" + When folder "merge-test-outside-perms" of user "user0" is shared with group "group1" with permissions 1 + And folder "merge-test-outside-perms" of user "user0" is shared with user "user1" with permissions 31 + Then as "user1" gets properties of folder "merge-test-outside-perms" with + |{http://owncloud.org/ns}permissions| + And the single response should contain a property "{http://owncloud.org/ns}permissions" with value "SRDNVCK" + And as "user1" the folder "merge-test-outside-perms (2)" does not exist + + Scenario: Merging shares for recipient when shared from outside with two groups + Given As an "admin" + And user "user0" exists + And user "user1" exists + And group "group1" exists + And group "group2" exists + And user "user1" belongs to group "group1" + And user "user1" belongs to group "group2" + And user "user0" created a folder "merge-test-outside-twogroups" + When folder "merge-test-outside-twogroups" of user "user0" is shared with group "group1" + And folder "merge-test-outside-twogroups" of user "user0" is shared with group "group2" + Then as "user1" the folder "merge-test-outside-twogroups" exists + And as "user1" the folder "merge-test-outside-twogroups (2)" does not exist + + Scenario: Merging shares for recipient when shared from outside with two groups with different permissions + Given As an "admin" + And user "user0" exists + And user "user1" exists + And group "group1" exists + And group "group2" exists + And user "user1" belongs to group "group1" + And user "user1" belongs to group "group2" + And user "user0" created a folder "merge-test-outside-twogroups-perms" + When folder "merge-test-outside-twogroups-perms" of user "user0" is shared with group "group1" with permissions 1 + And folder "merge-test-outside-twogroups-perms" of user "user0" is shared with group "group2" with permissions 31 + Then as "user1" gets properties of folder "merge-test-outside-twogroups-perms" with + |{http://owncloud.org/ns}permissions| + And the single response should contain a property "{http://owncloud.org/ns}permissions" with value "SRDNVCK" + And as "user1" the folder "merge-test-outside-twogroups-perms (2)" does not exist + + Scenario: Merging shares for recipient when shared from outside with two groups and member + Given As an "admin" + And user "user0" exists + And user "user1" exists + And group "group1" exists + And group "group2" exists + And user "user1" belongs to group "group1" + And user "user1" belongs to group "group2" + And user "user0" created a folder "merge-test-outside-twogroups-member-perms" + When folder "merge-test-outside-twogroups-member-perms" of user "user0" is shared with group "group1" with permissions 1 + And folder "merge-test-outside-twogroups-member-perms" of user "user0" is shared with group "group2" with permissions 31 + And folder "merge-test-outside-twogroups-member-perms" of user "user0" is shared with user "user1" with permissions 1 + Then as "user1" gets properties of folder "merge-test-outside-twogroups-member-perms" with + |{http://owncloud.org/ns}permissions| + And the single response should contain a property "{http://owncloud.org/ns}permissions" with value "SRDNVCK" + And as "user1" the folder "merge-test-outside-twogroups-member-perms (2)" does not exist + + Scenario: Merging shares for recipient when shared from inside with group + Given As an "admin" + And user "user0" exists + And group "group1" exists + And user "user0" belongs to group "group1" + And user "user0" created a folder "merge-test-inside-group" + When folder "/merge-test-inside-group" of user "user0" is shared with group "group1" + Then as "user0" the folder "merge-test-inside-group" exists + And as "user0" the folder "merge-test-inside-group (2)" does not exist + + Scenario: Merging shares for recipient when shared from inside with two groups + Given As an "admin" + And user "user0" exists + And group "group1" exists + And group "group2" exists + And user "user0" belongs to group "group1" + And user "user0" belongs to group "group2" + And user "user0" created a folder "merge-test-inside-twogroups" + When folder "merge-test-inside-twogroups" of user "user0" is shared with group "group1" + And folder "merge-test-inside-twogroups" of user "user0" is shared with group "group2" + Then as "user0" the folder "merge-test-inside-twogroups" exists + And as "user0" the folder "merge-test-inside-twogroups (2)" does not exist + And as "user0" the folder "merge-test-inside-twogroups (3)" does not exist + + Scenario: Merging shares for recipient when shared from inside with group with less permissions + Given As an "admin" + And user "user0" exists + And group "group1" exists + And group "group2" exists + And user "user0" belongs to group "group1" + And user "user0" belongs to group "group2" + And user "user0" created a folder "merge-test-inside-twogroups-perms" + When folder "merge-test-inside-twogroups-perms" of user "user0" is shared with group "group1" + And folder "merge-test-inside-twogroups-perms" of user "user0" is shared with group "group2" + Then as "user0" gets properties of folder "merge-test-inside-twogroups-perms" with + |{http://owncloud.org/ns}permissions| + And the single response should contain a property "{http://owncloud.org/ns}permissions" with value "RDNVCK" + And as "user0" the folder "merge-test-inside-twogroups-perms (2)" does not exist + And as "user0" the folder "merge-test-inside-twogroups-perms (3)" does not exist + From c3e7cf5030f96fb733eeb3bac985a49d55f6995a Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 13 Jul 2016 12:50:00 +0200 Subject: [PATCH 02/12] Added more tests for sharing's MountProvider --- apps/files_sharing/lib/mountprovider.php | 11 +- apps/files_sharing/tests/mountprovider.php | 146 +++++++++++++++++++++ 2 files changed, 156 insertions(+), 1 deletion(-) create mode 100644 apps/files_sharing/tests/mountprovider.php diff --git a/apps/files_sharing/lib/mountprovider.php b/apps/files_sharing/lib/mountprovider.php index b386c6704f9b..f9ff3f4df808 100644 --- a/apps/files_sharing/lib/mountprovider.php +++ b/apps/files_sharing/lib/mountprovider.php @@ -50,6 +50,15 @@ public function __construct(IConfig $config, ILogger $logger) { $this->logger = $logger; } + /** + * Return items shared with user + * + * @internal + */ + public function getItemsSharedWithUser($uid) { + // only here to make it mockable/testable + return \OCP\Share::getItemsSharedWithUser('file', $uid); + } /** * Get all mountpoints applicable for the user and check for shares where we need to update the etags @@ -59,7 +68,7 @@ public function __construct(IConfig $config, ILogger $logger) { * @return \OCP\Files\Mount\IMountPoint[] */ public function getMountsForUser(IUser $user, IStorageFactory $storageFactory) { - $shares = \OCP\Share::getItemsSharedWithUser('file', $user->getUID()); + $shares = $this->getItemsSharedWithUser($user->getUID()); $shares = array_filter($shares, function ($share) { return $share['permissions'] > 0; }); diff --git a/apps/files_sharing/tests/mountprovider.php b/apps/files_sharing/tests/mountprovider.php new file mode 100644 index 000000000000..72265552ae07 --- /dev/null +++ b/apps/files_sharing/tests/mountprovider.php @@ -0,0 +1,146 @@ + + * + * @copyright Copyright (c) 2016, 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 OCA\Files_Sharing\Tests; + +use OCA\Files_Sharing\MountProvider; +use OCP\Files\Storage\IStorageFactory; +use OCP\IConfig; +use OCP\ILogger; +use OCP\IUser; +use OCP\Share\IShare; +use OCP\Share\IManager; +use OCP\Files\Mount\IMountPoint; + +/** + * @group DB + */ +class MountProviderTest extends \Test\TestCase { + + /** @var MountProvider */ + private $provider; + + /** @var IConfig|\PHPUnit_Framework_MockObject_MockObject */ + private $config; + + /** @var IUser|\PHPUnit_Framework_MockObject_MockObject */ + private $user; + + /** @var IStorageFactory|\PHPUnit_Framework_MockObject_MockObject */ + private $loader; + + /** @var ILogger | \PHPUnit_Framework_MockObject_MockObject */ + private $logger; + + public function setUp() { + parent::setUp(); + + $this->config = $this->getMock('OCP\IConfig'); + $this->user = $this->getMock('OCP\IUser'); + $this->loader = $this->getMock('OCP\Files\Storage\IStorageFactory'); + $this->loader->expects($this->any()) + ->method('getInstance') + ->will($this->returnCallback(function($mountPoint, $class, $arguments) { + $storage = $this->getMockBuilder('OC\Files\Storage\Shared') + ->disableOriginalConstructor() + ->getMock(); + $storage->expects($this->any()) + ->method('getShare') + ->will($this->returnValue($arguments['share'])); + return $storage; + })); + $this->logger = $this->getMock('\OCP\ILogger'); + $this->logger->expects($this->never()) + ->method('error'); + + $this->provider = $this->getMockBuilder('OCA\Files_Sharing\MountProvider') + ->setMethods(['getItemsSharedWithUser']) + ->setConstructorArgs([$this->config, $this->logger]) + ->getMock(); + } + + private function makeMockShare($id, $nodeId, $owner = 'user2', $target = null, $permissions = 31, $shareType) { + return [ + 'id' => $id, + 'uid_owner' => $owner, + 'share_type' => $shareType, + 'item_type' => 'file', + 'file_target' => $target, + 'file_source' => $nodeId, + 'item_target' => null, + 'item_source' => $nodeId, + 'permissions' => $permissions, + 'stime' => time(), + 'token' => null, + 'expiration' => null, + ]; + } + + /** + * Tests excluding shares from the current view. This includes: + * - shares that were opted out of (permissions === 0) + * - shares with a group in which the owner is already in + */ + public function testExcludeShares() { + $userShares = [ + $this->makeMockShare(1, 100, 'user2', '/share2', 0, \OCP\Share::SHARE_TYPE_USER), + $this->makeMockShare(2, 100, 'user2', '/share2', 31, \OCP\Share::SHARE_TYPE_USER), + ]; + + $groupShares = [ + $this->makeMockShare(3, 100, 'user2', '/share2', 0, \OCP\Share::SHARE_TYPE_GROUP), + $this->makeMockShare(4, 100, 'user2', '/share4', 31, \OCP\Share::SHARE_TYPE_GROUP), + ]; + + $this->user->expects($this->any()) + ->method('getUID') + ->will($this->returnValue('user1')); + + $allShares = array_merge($userShares, $groupShares); + + $this->provider->expects($this->once()) + ->method('getItemsSharedWithUser') + ->with('user1') + ->will($this->returnValue($allShares)); + + $mounts = $this->provider->getMountsForUser($this->user, $this->loader); + + $this->assertCount(2, $mounts); + $this->assertInstanceOf('OCA\Files_Sharing\SharedMount', $mounts[0]); + $this->assertInstanceOf('OCA\Files_Sharing\SharedMount', $mounts[1]); + + $mountedShare1 = $mounts[0]->getShare(); + + $this->assertEquals('2', $mountedShare1['id']); + $this->assertEquals('user2', $mountedShare1['uid_owner']); + $this->assertEquals(100, $mountedShare1['file_source']); + $this->assertEquals('/share2', $mountedShare1['file_target']); + $this->assertEquals(31, $mountedShare1['permissions']); + + $mountedShare2 = $mounts[1]->getShare(); + $this->assertEquals('4', $mountedShare2['id']); + $this->assertEquals('user2', $mountedShare2['uid_owner']); + $this->assertEquals(100, $mountedShare2['file_source']); + $this->assertEquals('/share4', $mountedShare2['file_target']); + $this->assertEquals(31, $mountedShare2['permissions']); + } +} + From fd0f40383f4334ad5f9fb2ebc8893a7fdd653708 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 20 Jul 2016 16:48:54 +0200 Subject: [PATCH 03/12] Add repair step for unmerged shares --- lib/private/repair.php | 7 + lib/private/repair/repairunmergedshares.php | 326 ++++++++++++++ tests/lib/repair/repairunmergedsharestest.php | 409 ++++++++++++++++++ 3 files changed, 742 insertions(+) create mode 100644 lib/private/repair/repairunmergedshares.php create mode 100644 tests/lib/repair/repairunmergedsharestest.php diff --git a/lib/private/repair.php b/lib/private/repair.php index 0cbb43293e8c..0fa6f15a8aa0 100644 --- a/lib/private/repair.php +++ b/lib/private/repair.php @@ -49,6 +49,7 @@ use OC\Repair\SearchLuceneTables; use OC\Repair\UpdateOutdatedOcsIds; use OC\Repair\RepairInvalidShares; +use OC\Repair\RepairUnmergedShares; class Repair extends BasicEmitter { /** @@ -118,6 +119,12 @@ public static function getRepairSteps() { new RemoveGetETagEntries(\OC::$server->getDatabaseConnection()), new UpdateOutdatedOcsIds(\OC::$server->getConfig()), new RepairInvalidShares(\OC::$server->getConfig(), \OC::$server->getDatabaseConnection()), + new RepairUnmergedShares( + \OC::$server->getConfig(), + \OC::$server->getDatabaseConnection(), + \OC::$server->getUserManager(), + \OC::$server->getGroupManager() + ), new AvatarPermissions(\OC::$server->getDatabaseConnection()), new BrokenUpdaterRepair(), ]; diff --git a/lib/private/repair/repairunmergedshares.php b/lib/private/repair/repairunmergedshares.php new file mode 100644 index 000000000000..379e62ae65b1 --- /dev/null +++ b/lib/private/repair/repairunmergedshares.php @@ -0,0 +1,326 @@ + + * + * @copyright Copyright (c) 2016, 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\Repair; + +use OCP\Migration\IOutput; +use OC\Hooks\BasicEmitter; +use OC\Share\Constants; +use OCP\DB\QueryBuilder\IQueryBuilder; +use OCP\IConfig; +use OCP\IDBConnection; +use OCP\IUserManager; +use OCP\IUser; +use OCP\IGroupManager; +use OC\Share20\DefaultShareProvider; + +/** + * Repairs shares for which the received folder was not properly deduplicated. + * + * An unmerged share can for example happen when sharing a folder with the same + * user through multiple ways, like several groups and also directly, additionally + * to group shares. Since 9.0.0 these would create duplicate entries "folder (2)", + * one for every share. This repair step rearranges them so they only appear as a single + * folder. + */ +class RepairUnmergedShares extends BasicEmitter implements \OC\RepairStep { + + /** @var \OCP\IConfig */ + protected $config; + + /** @var \OCP\IDBConnection */ + protected $connection; + + /** @var IUserManager */ + protected $userManager; + + /** @var IGroupManager */ + protected $groupManager; + + /** @var IQueryBuilder */ + private $queryGetSharesWithUsers; + + /** @var IQueryBuilder */ + private $queryUpdateSharePermissionsAndTarget; + + /** @var IQueryBuilder */ + private $queryUpdateShareInBatch; + + /** + * @param \OCP\IConfig $config + * @param \OCP\IDBConnection $connection + */ + public function __construct( + IConfig $config, + IDBConnection $connection, + IUserManager $userManager, + IGroupManager $groupManager + ) { + $this->connection = $connection; + $this->config = $config; + $this->userManager = $userManager; + $this->groupManager = $groupManager; + } + + public function getName() { + return 'Repair unmerged shares'; + } + + /** + * Builds prepared queries for reuse + */ + private function buildPreparedQueries() { + /** + * Retrieve shares for a given user/group and share type + */ + $query = $this->connection->getQueryBuilder(); + $query + ->select('item_source', 'id', 'file_target', 'permissions', 'parent', 'share_type') + ->from('share') + ->where($query->expr()->eq('share_type', $query->createParameter('shareType'))) + ->andWhere($query->expr()->in('share_with', $query->createParameter('shareWiths'))) + ->andWhere($query->expr()->in('item_type', $query->createParameter('itemTypes'))) + ->orderBy('item_source', 'ASC') + ->addOrderBy('stime', 'ASC'); + + $this->queryGetSharesWithUsers = $query; + + /** + * Updates the file_target to the given value for all given share ids. + * + * This updates several shares in bulk which is faster than individually. + */ + $query = $this->connection->getQueryBuilder(); + $query->update('share') + ->set('file_target', $query->createParameter('file_target')) + ->where($query->expr()->in('id', $query->createParameter('ids'))); + + $this->queryUpdateShareInBatch = $query; + + /** + * Updates the share permissions and target path of a single share. + */ + $query = $this->connection->getQueryBuilder(); + $query->update('share') + ->set('permissions', $query->createParameter('permissions')) + ->set('file_target', $query->createParameter('file_target')) + ->where($query->expr()->eq('id', $query->createParameter('shareid'))); + + $this->queryUpdateSharePermissionsAndTarget = $query; + + } + + private function getSharesWithUser($shareType, $shareWiths) { + $groupedShares = []; + + $query = $this->queryGetSharesWithUsers; + $query->setParameter('shareWiths', $shareWiths, IQueryBuilder::PARAM_STR_ARRAY); + $query->setParameter('shareType', $shareType); + $query->setParameter('itemTypes', ['file', 'folder'], IQueryBuilder::PARAM_STR_ARRAY); + + $shares = $query->execute()->fetchAll(); + + // group by item_source + foreach ($shares as $share) { + if (!isset($groupedShares[$share['item_source']])) { + $groupedShares[$share['item_source']] = []; + } + $groupedShares[$share['item_source']][] = $share; + } + return $groupedShares; + } + + /** + * Fix the given received share represented by the set of group shares + * and matching sub shares + * + * @param array $groupShares group share entries + * @param array $subShares sub share entries + * + * @return boolean false if the share was not repaired, true if it was + */ + private function fixThisShare($groupShares, $subShares) { + $groupSharesById = []; + foreach ($groupShares as $groupShare) { + $groupSharesById[$groupShare['id']] = $groupShare; + } + + if ($this->isThisShareValid($groupSharesById, $subShares)) { + return false; + } + + $targetPath = $groupShares[0]['file_target']; + + // check whether the user opted out completely of all subshares + $optedOut = true; + foreach ($subShares as $subShare) { + if ((int)$subShare['permissions'] !== 0) { + $optedOut = false; + break; + } + } + + $shareIds = []; + foreach ($subShares as $subShare) { + // only if the user deleted some subshares but not all, adjust the permissions of that subshare + if (!$optedOut && (int)$subShare['permissions'] === 0 && (int)$subShare['share_type'] === DefaultShareProvider::SHARE_TYPE_USERGROUP) { + // set permissions from parent group share + $permissions = $groupSharesById[$subShare['parent']]['permissions']; + + // fix permissions and target directly + $query = $this->queryUpdateSharePermissionsAndTarget; + $query->setParameter('shareid', $subShare['id']); + $query->setParameter('file_target', $targetPath); + $query->setParameter('permissions', $permissions); + $query->execute(); + } else { + // gather share ids for bulk target update + if ($subShare['file_target'] !== $targetPath) { + $shareIds[] = (int)$subShare['id']; + } + } + } + + if (!empty($shareIds)) { + $query = $this->queryUpdateShareInBatch; + $query->setParameter('ids', $shareIds, IQueryBuilder::PARAM_INT_ARRAY); + $query->setParameter('file_target', $targetPath); + $query->execute(); + } + + return true; + } + + /** + * Checks whether the number of group shares is balanced with the child subshares. + * If all group shares have exactly one subshare, and the target of every subshare + * is the same, then the share is valid. + * If however there is a group share entry that has no matching subshare, it means + * we're in the bogus situation and the whole share must be repaired + * + * @param array $groupSharesById + * @param array $subShares + * + * @return true if the share is valid, false if it needs repair + */ + private function isThisShareValid($groupSharesById, $subShares) { + $foundTargets = []; + + // every group share needs to have exactly one matching subshare + foreach ($subShares as $subShare) { + $foundTargets[$subShare['file_target']] = true; + if (count($foundTargets) > 1) { + // not all the same target path value => invalid + return false; + } + if (isset($groupSharesById[$subShare['parent']])) { + // remove it from the list as we found it + unset($groupSharesById[$subShare['parent']]); + } + } + + // if we found one subshare per group entry, the set will be empty. + // If not empty, it means that one of the group shares did not have + // a matching subshare entry. + return empty($groupSharesById); + } + + /** + * Detect unmerged received shares and merge them properly + */ + private function fixUnmergedShares(IUser $user) { + $groups = $this->groupManager->getUserGroupIds($user); + if (empty($groups)) { + // user is in no groups, so can't have received group shares + return; + } + + $subSharesByItemSource = $this->getSharesWithUser(DefaultShareProvider::SHARE_TYPE_USERGROUP, [$user->getUID()]); + if (empty($subSharesByItemSource)) { + // nothing to repair for this user + return; + } + + $groupSharesByItemSource = $this->getSharesWithUser(Constants::SHARE_TYPE_GROUP, $groups); + if (empty($groupSharesByItemSource)) { + // shouldn't happen, those invalid shares must be cleant already by RepairInvalidShares + return; + } + + // because sometimes one wants to give the user more permissions than the group share + $userSharesByItemSource = $this->getSharesWithUser(Constants::SHARE_TYPE_USER, [$user->getUID()]); + + $fixed = 0; + foreach ($groupSharesByItemSource as $itemSource => $groupShares) { + if (!isset($subSharesByItemSource[$itemSource])) { + // no subshares for this item source, skip it + continue; + } + $subShares = $subSharesByItemSource[$itemSource]; + + if (isset($userSharesByItemSource[$itemSource])) { + // add it to the subshares to get a similar treatment + $subShares = array_merge($subShares, $userSharesByItemSource[$itemSource]); + } + + if ($this->fixThisShare($groupShares, $subShares)) { + $fixed++; + } + } + + if ($fixed > 0) { + $this->emit('\OC\Repair', 'info', array('Fixed ' . $fixed . ' share(s) that were not merged properly')); + } + } + + /** + * Count all the users + * + * @return int + */ + private function countUsers() { + $allCount = $this->userManager->countUsers(); + + $totalCount = 0; + foreach ($allCount as $backend => $count) { + $totalCount += $count; + } + + return $totalCount; + } + + public function run() { + $ocVersionFromBeforeUpdate = $this->config->getSystemValue('version', '0.0.0'); + if (version_compare($ocVersionFromBeforeUpdate, '9.0.4.1', '<')) { + // this situation was only possible between 9.0.0 and 9.0.4 included + + $function = function(IUser $user) { + $this->fixUnmergedShares($user); + }; + + $this->buildPreparedQueries(); + + $userCount = $this->countUsers(); + + $this->userManager->callForAllUsers($function); + } + } +} diff --git a/tests/lib/repair/repairunmergedsharestest.php b/tests/lib/repair/repairunmergedsharestest.php new file mode 100644 index 000000000000..20dabac2e6bc --- /dev/null +++ b/tests/lib/repair/repairunmergedsharestest.php @@ -0,0 +1,409 @@ + + * + * @copyright Copyright (c) 2016, 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 Test\Repair; + + +use OC\Repair\RepairUnmergedShares; +use OC\Share\Constants; +use OCP\Migration\IOutput; +use OCP\Migration\IRepairStep; +use Test\TestCase; +use OC\Share20\DefaultShareProvider; + +/** + * Tests for repairing invalid shares + * + * @group DB + * + * @see \OC\Repair\RepairUnmergedShares + */ +class RepairUnmergedSharesTest extends TestCase { + + /** @var IRepairStep */ + private $repair; + + /** @var \OCP\IDBConnection */ + private $connection; + + protected function setUp() { + parent::setUp(); + + $config = $this->getMockBuilder('OCP\IConfig') + ->disableOriginalConstructor() + ->getMock(); + $config->expects($this->any()) + ->method('getSystemValue') + ->with('version') + ->will($this->returnValue('9.0.3.0')); + + $this->connection = \OC::$server->getDatabaseConnection(); + $this->deleteAllShares(); + + $user1 = $this->getMock('\OCP\IUser'); + $user1->expects($this->any()) + ->method('getUID') + ->will($this->returnValue('user1')); + + $user2 = $this->getMock('\OCP\IUser'); + $user2->expects($this->any()) + ->method('getUID') + ->will($this->returnValue('user2')); + + $users = [$user1, $user2]; + + $groupManager = $this->getMock('\OCP\IGroupManager'); + $groupManager->expects($this->any()) + ->method('getUserGroupIds') + ->will($this->returnValueMap([ + // owner + [$user1, ['samegroup1', 'samegroup2']], + // recipient + [$user2, ['recipientgroup1', 'recipientgroup2']], + ])); + + $userManager = $this->getMock('\OCP\IUserManager'); + $userManager->expects($this->once()) + ->method('countUsers') + ->will($this->returnValue([2])); + $userManager->expects($this->once()) + ->method('callForAllUsers') + ->will($this->returnCallback(function(\Closure $closure) use ($users) { + foreach ($users as $user) { + $closure($user); + } + })); + + /** @var \OCP\IConfig $config */ + $this->repair = new RepairUnmergedShares($config, $this->connection, $userManager, $groupManager); + } + + protected function tearDown() { + $this->deleteAllShares(); + + parent::tearDown(); + } + + protected function deleteAllShares() { + $qb = $this->connection->getQueryBuilder(); + $qb->delete('share')->execute(); + } + + private function createShare($type, $sourceId, $recipient, $targetName, $permissions, $parentId = null) { + $qb = $this->connection->getQueryBuilder(); + $values = [ + 'share_type' => $qb->expr()->literal($type), + 'share_with' => $qb->expr()->literal($recipient), + 'uid_owner' => $qb->expr()->literal('user1'), + 'item_type' => $qb->expr()->literal('folder'), + 'item_source' => $qb->expr()->literal($sourceId), + 'item_target' => $qb->expr()->literal('/' . $sourceId), + 'file_source' => $qb->expr()->literal($sourceId), + 'file_target' => $qb->expr()->literal($targetName), + 'permissions' => $qb->expr()->literal($permissions), + 'stime' => $qb->expr()->literal(time()), + ]; + if ($parentId !== null) { + $values['parent'] = $qb->expr()->literal($parentId); + } + $qb->insert('share') + ->values($values) + ->execute(); + + return $this->connection->lastInsertId('*PREFIX*share'); + } + + private function getShareById($id) { + $query = $this->connection->getQueryBuilder(); + $results = $query + ->select('*') + ->from('share') + ->where($query->expr()->eq('id', $query->expr()->literal($id))) + ->execute() + ->fetchAll(); + + if (!empty($results)) { + return $results[0]; + } + return null; + } + + public function sharesDataProvider() { + /** + * For all these test cases we have the following situation: + * + * - "user1" is the share owner + * - "user2" is the recipient, and member of "recipientgroup1" and "recipientgroup2" + * - "user1" is member of "samegroup1", "samegroup2" for same group tests + */ + return [ + [ + // #0 legitimate share: + // - outsider shares with group1, group2 + // - recipient renamed, resulting in subshares + // - one subshare for each group share + // - targets of subshare all match + [ + [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup1', '/test', 31], + [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup2', '/test', 31], + // child of the previous ones + [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test renamed', 31, 0], + // child of the previous ones + [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test renamed', 31, 1], + // different unrelated share + [Constants::SHARE_TYPE_GROUP, 456, 'recipientgroup1', '/test (4)', 31], + ], + [ + ['/test', 31], + ['/test', 31], + // leave them alone + ['/test renamed', 31], + ['/test renamed', 31], + // leave unrelated alone + ['/test (4)', 31], + ] + ], + [ + // #1 broken share: + // - outsider shares with group1, group2 + // - only one subshare for two group shares + [ + [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup1', '/test', 31], + [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup2', '/test', 31], + // child of the previous one + [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test (2)', 31, 1], + // different unrelated share + [Constants::SHARE_TYPE_GROUP, 456, 'recipientgroup1', '/test (4)', 31], + ], + [ + ['/test', 31], + ['/test', 31], + ['/test', 31], + // leave unrelated alone + ['/test (4)', 31], + ] + ], + [ + // #2 bogus share + // - outsider shares with group1, group2 + // - one subshare for each group share + // - but the targets do not match when grouped + [ + [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup1', '/test', 31], + [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup2', '/test', 31], + // child of the previous ones + [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test (2)', 31, 0], + [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test (3)', 31, 1], + // different unrelated share + [Constants::SHARE_TYPE_GROUP, 456, 'recipientgroup1', '/test (4)', 31], + ], + [ + ['/test', 31], + ['/test', 31], + // reset to original name + ['/test', 31], + ['/test', 31], + // leave unrelated alone + ['/test (4)', 31], + ] + ], + [ + // #3 bogus share + // - outsider shares with group1, group2 + // - one subshare for each group share + // - first subshare not renamed (as in real world scenario) + // - but the targets do not match when grouped + [ + [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup1', '/test', 31], + [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup2', '/test', 31], + // child of the previous ones + [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test', 31, 0], + [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test (2)', 31, 1], + // different unrelated share + [Constants::SHARE_TYPE_GROUP, 456, 'recipientgroup1', '/test (4)', 31], + ], + [ + ['/test', 31], + ['/test', 31], + // reset to original name + ['/test', 31], + ['/test', 31], + // leave unrelated alone + ['/test (4)', 31], + ] + ], + [ + // #4 bogus share: + // - outsider shares with group1, group2 + // - one subshare for each group share + // - non-matching targets + // - recipient deletes one duplicate (unshare from self, permissions 0) + [ + [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup1', '/test', 31], + [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup2', '/test', 15], + // child of the previous ones + [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test (2)', 0, 0], + [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test (3)', 15, 1], + // different unrelated share + [Constants::SHARE_TYPE_GROUP, 456, 'recipientgroup1', '/test (4)', 31], + ], + [ + ['/test', 31], + ['/test', 15], + // subshares repaired and permissions restored to the max allowed + ['/test', 31], + ['/test', 15], + // leave unrelated alone + ['/test (4)', 31], + ] + ], + [ + // #5 bogus share: + // - outsider shares with group1, group2 + // - one subshare for each group share + // - non-matching targets + // - recipient deletes ALL duplicates (unshare from self, permissions 0) + [ + [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup1', '/test', 31], + [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup2', '/test', 15], + // child of the previous ones + [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test (2)', 0, 0], + [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test (3)', 0, 1], + // different unrelated share + [Constants::SHARE_TYPE_GROUP, 456, 'recipientgroup1', '/test (4)', 31], + ], + [ + ['/test', 31], + ['/test', 15], + // subshares target repaired but left "deleted" as it was the user's choice + ['/test', 0], + ['/test', 0], + // leave unrelated alone + ['/test (4)', 31], + ] + ], + [ + // #6 bogus share: + // - outsider shares with group1, group2 and also user2 + // - one subshare for each group share + // - one extra share entry for direct share to user2 + // - non-matching targets + // - user share has more permissions + [ + [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup1', '/test', 1], + [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup2', '/test', 15], + // child of the previous ones + [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test (2)', 1, 0], + [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test (3)', 15, 1], + [Constants::SHARE_TYPE_USER, 123, 'user2', '/test (4)', 31], + // different unrelated share + [Constants::SHARE_TYPE_GROUP, 456, 'recipientgroup1', '/test (5)', 31], + ], + [ + ['/test', 1], + ['/test', 15], + // subshares repaired + ['/test', 1], + ['/test', 15], + ['/test', 31], + // leave unrelated alone + ['/test (5)', 31], + ] + ], + [ + // #7 legitimate share with own group: + // - insider shares with both groups the user is already in + // - no subshares in this case + [ + [Constants::SHARE_TYPE_GROUP, 123, 'samegroup1', '/test', 31], + [Constants::SHARE_TYPE_GROUP, 123, 'samegroup2', '/test', 31], + // different unrelated share + [Constants::SHARE_TYPE_GROUP, 456, 'recipientgroup1', '/test (4)', 31], + ], + [ + // leave all alone + ['/test', 31], + ['/test', 31], + // leave unrelated alone + ['/test (4)', 31], + ] + ], + [ + // #7 legitimate shares: + // - group share with same group + // - group share with other group + // - user share where recipient renamed + // - user share where recipient did not rename + [ + [Constants::SHARE_TYPE_GROUP, 123, 'samegroup1', '/test', 31], + [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup1', '/test', 31], + [Constants::SHARE_TYPE_USER, 123, 'user3', '/test legit rename', 31], + [Constants::SHARE_TYPE_USER, 123, 'user4', '/test', 31], + // different unrelated share + [Constants::SHARE_TYPE_GROUP, 456, 'recipientgroup1', '/test (4)', 31], + ], + [ + // leave all alone + ['/test', 31], + ['/test', 31], + ['/test legit rename', 31], + ['/test', 31], + // leave unrelated alone + ['/test (4)', 31], + ] + ], + ]; + } + + /** + * Test merge shares from group shares + * + * @dataProvider sharesDataProvider + */ + public function testMergeGroupShares($shares, $expectedShares) { + $shareIds = []; + + foreach ($shares as $share) { + // if parent + if (isset($share[5])) { + // adjust to real id + $share[5] = $shareIds[$share[5]]; + } else { + $share[5] = null; + } + $shareIds[] = $this->createShare($share[0], $share[1], $share[2], $share[3], $share[4], $share[5]); + } + + /** @var IOutput | \PHPUnit_Framework_MockObject_MockObject $outputMock */ + $outputMock = $this->getMockBuilder('\OCP\Migration\IOutput') + ->disableOriginalConstructor() + ->getMock(); + + $this->repair->run($outputMock); + + foreach ($expectedShares as $index => $expectedShare) { + $share = $this->getShareById($shareIds[$index]); + $this->assertEquals($expectedShare[0], $share['file_target']); + $this->assertEquals($expectedShare[1], $share['permissions']); + } + } +} + From 5c15a94c5466a24e13f497b0282ec86250171348 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 13 Jul 2016 15:46:02 +0200 Subject: [PATCH 04/12] Group incoming shares for resharing in JS --- core/js/shareitemmodel.js | 29 +++++++++++++++- core/js/tests/specs/shareitemmodelSpec.js | 42 +++++++++++++++++++++++ 2 files changed, 70 insertions(+), 1 deletion(-) diff --git a/core/js/shareitemmodel.js b/core/js/shareitemmodel.js index 292230d26d55..87574a02d013 100644 --- a/core/js/shareitemmodel.js +++ b/core/js/shareitemmodel.js @@ -601,6 +601,33 @@ } }, + /** + * Group reshares into a single super share element. + * Does this by finding the most precise share and + * combines the permissions to be the most permissive. + * + * @param {Array} reshares + * @return {Object} reshare + */ + _groupReshares: function(reshares) { + if (!reshares || !reshares.length) { + return false; + } + + var superShare = reshares.shift(); + var combinedPermissions = superShare.permissions; + _.each(reshares, function(reshare) { + // use share have higher priority than group share + if (reshare.share_type === OC.Share.SHARE_TYPE_USER && superShare.share_type === OC.Share.SHARE_TYPE_GROUP) { + superShare = reshare; + } + combinedPermissions |= reshare.permissions; + }); + + superShare.permissions = combinedPermissions; + return superShare; + }, + fetch: function() { var model = this; this.trigger('request', this); @@ -618,7 +645,7 @@ var reshare = false; if (data2[0].ocs.data.length) { - reshare = data2[0].ocs.data[0]; + reshare = model._groupReshares(data2[0].ocs.data); } model.set(model.parse({ diff --git a/core/js/tests/specs/shareitemmodelSpec.js b/core/js/tests/specs/shareitemmodelSpec.js index 8c9560d2646d..9d9001dc9e8f 100644 --- a/core/js/tests/specs/shareitemmodelSpec.js +++ b/core/js/tests/specs/shareitemmodelSpec.js @@ -181,6 +181,48 @@ describe('OC.Share.ShareItemModel', function() { // TODO: check more attributes }); + it('groups reshare info into a single item', function() { + /* jshint camelcase: false */ + fetchReshareDeferred.resolve(makeOcsResponse([ + { + id: '1', + share_type: OC.Share.SHARE_TYPE_USER, + uid_owner: 'owner', + displayname_owner: 'Owner', + share_with: 'root', + permissions: 1 + }, + { + id: '2', + share_type: OC.Share.SHARE_TYPE_GROUP, + uid_owner: 'owner', + displayname_owner: 'Owner', + share_with: 'group1', + permissions: 15 + }, + { + id: '3', + share_type: OC.Share.SHARE_TYPE_GROUP, + uid_owner: 'owner', + displayname_owner: 'Owner', + share_with: 'group1', + permissions: 17 + } + ])); + fetchSharesDeferred.resolve(makeOcsResponse([])); + + OC.currentUser = 'root'; + + model.fetch(); + + var reshare = model.get('reshare'); + // max permissions + expect(reshare.permissions).toEqual(31); + // user share has higher priority + expect(reshare.share_type).toEqual(OC.Share.SHARE_TYPE_USER); + expect(reshare.share_with).toEqual('root'); + expect(reshare.id).toEqual('1'); + }); it('does not parse link share when for a different file', function() { /* jshint camelcase: false */ fetchReshareDeferred.resolve(makeOcsResponse([])); From a39f3ebae251d26e8455e1afa98a65cb91481905 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 22 Jul 2016 07:59:15 +0200 Subject: [PATCH 05/12] Fix grouping of received shares in MountProvider Added flag to enforce grouping of received shares even when the method is called for a user different than the current one. This can happen at sharing time whenever the recipient's FS is being setup from the sharer's call. This fixes duplicated received folders for new shares. --- apps/files_sharing/lib/mountprovider.php | 2 +- lib/private/share/share.php | 17 +++++++++++------ lib/public/share.php | 5 +++-- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/apps/files_sharing/lib/mountprovider.php b/apps/files_sharing/lib/mountprovider.php index f9ff3f4df808..b99cafce4c1a 100644 --- a/apps/files_sharing/lib/mountprovider.php +++ b/apps/files_sharing/lib/mountprovider.php @@ -57,7 +57,7 @@ public function __construct(IConfig $config, ILogger $logger) { */ public function getItemsSharedWithUser($uid) { // only here to make it mockable/testable - return \OCP\Share::getItemsSharedWithUser('file', $uid); + return \OCP\Share::getItemsSharedWithUser('file', $uid, \OCP\Share::FORMAT_NONE, null, -1, false, true); } /** diff --git a/lib/private/share/share.php b/lib/private/share/share.php index d6bcbf902d54..6c970f556437 100644 --- a/lib/private/share/share.php +++ b/lib/private/share/share.php @@ -328,12 +328,13 @@ public static function getItemsSharedWith($itemType, $format = self::FORMAT_NONE * @param mixed $parameters (optional) * @param int $limit Number of items to return (optional) Returns all by default * @param boolean $includeCollections (optional) + * @param boolean $forceGrouping (optional) force grouping * @return mixed Return depends on format */ public static function getItemsSharedWithUser($itemType, $user, $format = self::FORMAT_NONE, - $parameters = null, $limit = -1, $includeCollections = false) { + $parameters = null, $limit = -1, $includeCollections = false, $forceGrouping = false) { return self::getItems($itemType, null, self::$shareTypeUserAndGroups, $user, null, $format, - $parameters, $limit, $includeCollections); + $parameters, $limit, $includeCollections, false, true, $forceGrouping); } /** @@ -1643,7 +1644,8 @@ public static function getSharedItemsOwners($user, $type, $includeCollections = * @param int $limit Number of items to return, -1 to return all matches (optional) * @param boolean $includeCollections Include collection item types (optional) * @param boolean $itemShareWithBySource (optional) - * @param boolean $checkExpireDate + * @param boolean $checkExpireDate (optional) + * @param boolean $forceGrouping (optional) force grouping * @return array * * See public functions getItem(s)... for parameter usage @@ -1651,7 +1653,9 @@ public static function getSharedItemsOwners($user, $type, $includeCollections = */ public static function getItems($itemType, $item = null, $shareType = null, $shareWith = null, $uidOwner = null, $format = self::FORMAT_NONE, $parameters = null, $limit = -1, - $includeCollections = false, $itemShareWithBySource = false, $checkExpireDate = true) { + $includeCollections = false, $itemShareWithBySource = false, $checkExpireDate = true, + $forceGrouping = false + ) { if (!self::isEnabled()) { return array(); } @@ -1928,8 +1932,9 @@ public static function getItems($itemType, $item = null, $shareType = null, $sha } - // group items if we are looking for items shared with the current user - if (isset($shareWith) && $shareWith === \OCP\User::getUser()) { + // group items if we are looking for items shared with the current user, + // or forceGrouping was set because the caller does always want grouped results + if (isset($shareWith) && ($shareWith === \OCP\User::getUser() || $forceGrouping)) { $items = self::groupItems($items, $itemType); } diff --git a/lib/public/share.php b/lib/public/share.php index e21d82bf62c5..010e5c9bf4d5 100644 --- a/lib/public/share.php +++ b/lib/public/share.php @@ -114,13 +114,14 @@ public static function getItemsSharedWith($itemType, $format = self::FORMAT_NONE * @param mixed $parameters (optional) * @param int $limit Number of items to return (optional) Returns all by default * @param bool $includeCollections (optional) + * @param bool $forceGrouping (optional) force grouping received shares * @return mixed Return depends on format * @since 7.0.0 */ public static function getItemsSharedWithUser($itemType, $user, $format = self::FORMAT_NONE, - $parameters = null, $limit = -1, $includeCollections = false) { + $parameters = null, $limit = -1, $includeCollections = false, $forceGrouping = false) { - return \OC\Share\Share::getItemsSharedWithUser($itemType, $user, $format, $parameters, $limit, $includeCollections); + return \OC\Share\Share::getItemsSharedWithUser($itemType, $user, $format, $parameters, $limit, $includeCollections, $forceGrouping); } /** From b194b9f408851c7fd52449c69ea1bb06d126987f Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 22 Jul 2016 08:03:05 +0200 Subject: [PATCH 06/12] Adjust RepairUnmergedShares version check --- lib/private/repair/repairunmergedshares.php | 2 +- version.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/private/repair/repairunmergedshares.php b/lib/private/repair/repairunmergedshares.php index 379e62ae65b1..5027dddce929 100644 --- a/lib/private/repair/repairunmergedshares.php +++ b/lib/private/repair/repairunmergedshares.php @@ -309,7 +309,7 @@ private function countUsers() { public function run() { $ocVersionFromBeforeUpdate = $this->config->getSystemValue('version', '0.0.0'); - if (version_compare($ocVersionFromBeforeUpdate, '9.0.4.1', '<')) { + if (version_compare($ocVersionFromBeforeUpdate, '9.0.4.2', '<')) { // this situation was only possible between 9.0.0 and 9.0.4 included $function = function(IUser $user) { diff --git a/version.php b/version.php index 9f7831e0cd76..5f2c604be328 100644 --- a/version.php +++ b/version.php @@ -26,7 +26,7 @@ // We only can count up. The 4. digit is only for the internal patchlevel to trigger DB upgrades // between betas, final and RCs. This is _not_ the public version number. Reset minor/patchlevel // when updating major/minor version number. -$OC_Version = array(9, 0, 4, 1); +$OC_Version = array(9, 0, 4, 2); // The human readable string $OC_VersionString = '9.0.4'; From 2d463d3991da9e7c26156dd42cff46a330669e20 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 22 Jul 2016 10:08:05 +0200 Subject: [PATCH 07/12] Fix RepairUnmergedShares to not skip valid repair cases The repair step was a bit overeager to skip repairing so it missed the case where a group share exists without subshares but with an additional direct user share. --- lib/private/repair/repairunmergedshares.php | 27 +++++++----- tests/lib/repair/repairunmergedsharestest.php | 44 ++++++++++++++++++- 2 files changed, 58 insertions(+), 13 deletions(-) diff --git a/lib/private/repair/repairunmergedshares.php b/lib/private/repair/repairunmergedshares.php index 5027dddce929..77bb03ad462a 100644 --- a/lib/private/repair/repairunmergedshares.php +++ b/lib/private/repair/repairunmergedshares.php @@ -158,6 +158,10 @@ private function getSharesWithUser($shareType, $shareWiths) { * @return boolean false if the share was not repaired, true if it was */ private function fixThisShare($groupShares, $subShares) { + if (empty($subShares)) { + return false; + } + $groupSharesById = []; foreach ($groupShares as $groupShare) { $groupSharesById[$groupShare['id']] = $groupShare; @@ -253,28 +257,29 @@ private function fixUnmergedShares(IUser $user) { return; } + // get all subshares grouped by item source $subSharesByItemSource = $this->getSharesWithUser(DefaultShareProvider::SHARE_TYPE_USERGROUP, [$user->getUID()]); - if (empty($subSharesByItemSource)) { - // nothing to repair for this user + + // because sometimes one wants to give the user more permissions than the group share + $userSharesByItemSource = $this->getSharesWithUser(Constants::SHARE_TYPE_USER, [$user->getUID()]); + + if (empty($subSharesByItemSource) && empty($userSharesByItemSource)) { + // nothing to repair for this user, no need to do extra queries return; } $groupSharesByItemSource = $this->getSharesWithUser(Constants::SHARE_TYPE_GROUP, $groups); - if (empty($groupSharesByItemSource)) { - // shouldn't happen, those invalid shares must be cleant already by RepairInvalidShares + if (empty($groupSharesByItemSource) && empty($userSharesByItemSource)) { + // nothing to repair for this user return; } - // because sometimes one wants to give the user more permissions than the group share - $userSharesByItemSource = $this->getSharesWithUser(Constants::SHARE_TYPE_USER, [$user->getUID()]); - $fixed = 0; foreach ($groupSharesByItemSource as $itemSource => $groupShares) { - if (!isset($subSharesByItemSource[$itemSource])) { - // no subshares for this item source, skip it - continue; + $subShares = []; + if (isset($subSharesByItemSource[$itemSource])) { + $subShares = $subSharesByItemSource[$itemSource]; } - $subShares = $subSharesByItemSource[$itemSource]; if (isset($userSharesByItemSource[$itemSource])) { // add it to the subshares to get a similar treatment diff --git a/tests/lib/repair/repairunmergedsharestest.php b/tests/lib/repair/repairunmergedsharestest.php index 20dabac2e6bc..fe9b3e5b96f3 100644 --- a/tests/lib/repair/repairunmergedsharestest.php +++ b/tests/lib/repair/repairunmergedsharestest.php @@ -329,7 +329,28 @@ public function sharesDataProvider() { ] ], [ - // #7 legitimate share with own group: + // #7 bogus share: + // - outsider shares with group1 and also user2 + // - no subshare at all + // - one extra share entry for direct share to user2 + // - non-matching targets + // - user share has more permissions + [ + [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup1', '/test', 1], + [Constants::SHARE_TYPE_USER, 123, 'user2', '/test (2)', 31], + // different unrelated share + [Constants::SHARE_TYPE_GROUP, 456, 'recipientgroup1', '/test (5)', 31], + ], + [ + ['/test', 1], + // user share repaired + ['/test', 31], + // leave unrelated alone + ['/test (5)', 31], + ] + ], + [ + // #8 legitimate share with own group: // - insider shares with both groups the user is already in // - no subshares in this case [ @@ -347,7 +368,7 @@ public function sharesDataProvider() { ] ], [ - // #7 legitimate shares: + // #9 legitimate shares: // - group share with same group // - group share with other group // - user share where recipient renamed @@ -370,6 +391,25 @@ public function sharesDataProvider() { ['/test (4)', 31], ] ], + [ + // #10 legitimate share: + // - outsider shares with group and user directly with different permissions + // - no subshares + // - same targets + [ + [Constants::SHARE_TYPE_GROUP, 123, 'samegroup1', '/test', 1], + [Constants::SHARE_TYPE_USER, 123, 'user3', '/test', 31], + // different unrelated share + [Constants::SHARE_TYPE_GROUP, 456, 'recipientgroup1', '/test (4)', 31], + ], + [ + // leave all alone + ['/test', 1], + ['/test', 31], + // leave unrelated alone + ['/test (4)', 31], + ] + ], ]; } From f897e19c946c45e5b844647ddfdffd826b001539 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 22 Jul 2016 11:42:21 +0200 Subject: [PATCH 08/12] Add integration tests for double shares with rename in between --- build/integration/features/sharing-v1.feature | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/build/integration/features/sharing-v1.feature b/build/integration/features/sharing-v1.feature index 5f5e9fb21dc5..40eb73cb1070 100644 --- a/build/integration/features/sharing-v1.feature +++ b/build/integration/features/sharing-v1.feature @@ -721,3 +721,33 @@ Feature: sharing And as "user0" the folder "merge-test-inside-twogroups-perms (2)" does not exist And as "user0" the folder "merge-test-inside-twogroups-perms (3)" does not exist + Scenario: Merging shares for recipient when shared from outside with group then user and recipient renames in between + Given As an "admin" + And user "user0" exists + And user "user1" exists + And group "group1" exists + And user "user1" belongs to group "group1" + And user "user0" created a folder "merge-test-outside-groups-renamebeforesecondshare" + When folder "merge-test-outside-groups-renamebeforesecondshare" of user "user0" is shared with group "group1" + And User "user1" moved folder "/merge-test-outside-groups-renamebeforesecondshare" to "/merge-test-outside-groups-renamebeforesecondshare-renamed" + And folder "merge-test-outside-groups-renamebeforesecondshare" of user "user0" is shared with user "user1" + Then as "user1" gets properties of folder "merge-test-outside-groups-renamebeforesecondshare-renamed" with + |{http://owncloud.org/ns}permissions| + And the single response should contain a property "{http://owncloud.org/ns}permissions" with value "SRDNVCK" + And as "user1" the folder "merge-test-outside-groups-renamebeforesecondshare" does not exist + + Scenario: Merging shares for recipient when shared from outside with user then group and recipient renames in between + Given As an "admin" + And user "user0" exists + And user "user1" exists + And group "group1" exists + And user "user1" belongs to group "group1" + And user "user0" created a folder "merge-test-outside-groups-renamebeforesecondshare" + When folder "merge-test-outside-groups-renamebeforesecondshare" of user "user0" is shared with user "user1" + And User "user1" moved folder "/merge-test-outside-groups-renamebeforesecondshare" to "/merge-test-outside-groups-renamebeforesecondshare-renamed" + And folder "merge-test-outside-groups-renamebeforesecondshare" of user "user0" is shared with group "group1" + Then as "user1" gets properties of folder "merge-test-outside-groups-renamebeforesecondshare-renamed" with + |{http://owncloud.org/ns}permissions| + And the single response should contain a property "{http://owncloud.org/ns}permissions" with value "SRDNVCK" + And as "user1" the folder "merge-test-outside-groups-renamebeforesecondshare" does not exist + From 2e04cc8e6c4749df88f21326daac6eb6634ec3b3 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 22 Jul 2016 16:39:02 +0200 Subject: [PATCH 09/12] Ignore file_target when grouping shares --- lib/private/share/share.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/private/share/share.php b/lib/private/share/share.php index 6c970f556437..3804d656dbac 100644 --- a/lib/private/share/share.php +++ b/lib/private/share/share.php @@ -2068,10 +2068,13 @@ protected static function groupItems($items, $itemType) { foreach ($items as $item) { $grouped = false; foreach ($result as $key => $r) { - // for file/folder shares we need to compare file_source, otherwise we compare item_source + // for file/folder shares we need to compare file_source + // since we only want a single received target for the same file_source + // + // otherwise we compare item_source // only group shares if they already point to the same target, otherwise the file where shared // before grouping of shares was added. In this case we don't group them toi avoid confusions - if (( $fileSharing && $item['file_source'] === $r['file_source'] && $item['file_target'] === $r['file_target']) || + if (( $fileSharing && $item['file_source'] === $r['file_source']) || (!$fileSharing && $item['item_source'] === $r['item_source'] && $item['item_target'] === $r['item_target'])) { // add the first item to the list of grouped shares if (!isset($result[$key]['grouped'])) { @@ -2087,7 +2090,6 @@ protected static function groupItems($items, $itemType) { if (!$grouped) { $result[] = $item; } - } return $result; From ca1043e4a97cc6d9f622d174ea529f667e962a9b Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 12 Aug 2016 10:56:51 +0200 Subject: [PATCH 10/12] Don't bother repairing unmerged shares when coming from OC 8.2 This would slow down the upgrade needlessly as there is no repair to be done. --- lib/private/repair/repairunmergedshares.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/repair/repairunmergedshares.php b/lib/private/repair/repairunmergedshares.php index 77bb03ad462a..495d8729f734 100644 --- a/lib/private/repair/repairunmergedshares.php +++ b/lib/private/repair/repairunmergedshares.php @@ -314,7 +314,7 @@ private function countUsers() { public function run() { $ocVersionFromBeforeUpdate = $this->config->getSystemValue('version', '0.0.0'); - if (version_compare($ocVersionFromBeforeUpdate, '9.0.4.2', '<')) { + if (version_compare($ocVersionFromBeforeUpdate, '9.0.0.0', '>=') && version_compare($ocVersionFromBeforeUpdate, '9.0.4.2', '<')) { // this situation was only possible between 9.0.0 and 9.0.4 included $function = function(IUser $user) { From f68dd4d821a7c2ab19c0f70c07a0e586b1975cbd Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 12 Aug 2016 11:44:34 +0200 Subject: [PATCH 11/12] Improve file_target finding logic when repairing unmerged shares Pick the most recent subshare that has no parenthesis from duplication which should match whichever name the user picked last. If all subshares have duplicate parenthesis names, use the least recent group share's target instead. --- lib/private/repair/repairunmergedshares.php | 40 ++++++++++- tests/lib/repair/repairunmergedsharestest.php | 66 ++++++++++++++++--- 2 files changed, 96 insertions(+), 10 deletions(-) diff --git a/lib/private/repair/repairunmergedshares.php b/lib/private/repair/repairunmergedshares.php index 495d8729f734..723a153aee3e 100644 --- a/lib/private/repair/repairunmergedshares.php +++ b/lib/private/repair/repairunmergedshares.php @@ -148,6 +148,44 @@ private function getSharesWithUser($shareType, $shareWiths) { return $groupedShares; } + /** + * Decide on the best target name based on all group shares and subshares, + * goal is to increase the likeliness that the chosen name matches what + * the user is expecting. + * + * For this, we discard the entries with parenthesis "(2)". + * In case the user also renamed the duplicates to a legitimate name, this logic + * will still pick the most recent one as it's the one the user is most likely to + * remember renaming. + * + * If no suitable subshare is found, use the least recent group share instead. + * + * @param array $groupShares group share entries + * @param array $subShares sub share entries, sorted by stime + * + * @return string chosen target name + */ + private function findBestTargetName($groupShares, $subShares) { + $pickedShare = null; + // note subShares are sorted by stime from oldest to newest + foreach ($subShares as $subShare) { + // skip entries that have parenthesis with numbers + if (preg_match('/\([0-9]*\)/', $subShare['file_target']) === 1) { + continue; + } + // pick any share found that would match, the last being the most recent + $pickedShare = $subShare; + } + + // no suitable subshare found + if ($pickedShare === null) { + // use least recent group share target instead + $pickedShare = $groupShares[0]; + } + + return $pickedShare['file_target']; + } + /** * Fix the given received share represented by the set of group shares * and matching sub shares @@ -171,7 +209,7 @@ private function fixThisShare($groupShares, $subShares) { return false; } - $targetPath = $groupShares[0]['file_target']; + $targetPath = $this->findBestTargetName($groupShares, $subShares); // check whether the user opted out completely of all subshares $optedOut = true; diff --git a/tests/lib/repair/repairunmergedsharestest.php b/tests/lib/repair/repairunmergedsharestest.php index fe9b3e5b96f3..9fe9a0738810 100644 --- a/tests/lib/repair/repairunmergedsharestest.php +++ b/tests/lib/repair/repairunmergedsharestest.php @@ -204,7 +204,7 @@ public function sharesDataProvider() { [ // #2 bogus share // - outsider shares with group1, group2 - // - one subshare for each group share + // - one subshare for each group share, both with parenthesis // - but the targets do not match when grouped [ [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup1', '/test', 31], @@ -218,7 +218,7 @@ public function sharesDataProvider() { [ ['/test', 31], ['/test', 31], - // reset to original name + // reset to original name as the sub-names have parenthesis ['/test', 31], ['/test', 31], // leave unrelated alone @@ -228,6 +228,54 @@ public function sharesDataProvider() { [ // #3 bogus share // - outsider shares with group1, group2 + // - one subshare for each group share, both renamed manually + // - but the targets do not match when grouped + [ + [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup1', '/test', 31], + [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup2', '/test', 31], + // child of the previous ones + [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test_renamed (1 legit paren)', 31, 0], + [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test_renamed (2 legit paren)', 31, 1], + // different unrelated share + [Constants::SHARE_TYPE_GROUP, 456, 'recipientgroup1', '/test (4)', 31], + ], + [ + ['/test', 31], + ['/test', 31], + // reset to less recent subshare name + ['/test_renamed (2 legit paren)', 31], + ['/test_renamed (2 legit paren)', 31], + // leave unrelated alone + ['/test (4)', 31], + ] + ], + [ + // #4 bogus share + // - outsider shares with group1, group2 + // - one subshare for each group share, one with parenthesis + // - but the targets do not match when grouped + [ + [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup1', '/test', 31], + [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup2', '/test', 31], + // child of the previous ones + [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test (2)', 31, 0], + [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test_renamed', 31, 1], + // different unrelated share + [Constants::SHARE_TYPE_GROUP, 456, 'recipientgroup1', '/test (4)', 31], + ], + [ + ['/test', 31], + ['/test', 31], + // reset to less recent subshare name but without parenthesis + ['/test_renamed', 31], + ['/test_renamed', 31], + // leave unrelated alone + ['/test (4)', 31], + ] + ], + [ + // #5 bogus share + // - outsider shares with group1, group2 // - one subshare for each group share // - first subshare not renamed (as in real world scenario) // - but the targets do not match when grouped @@ -251,7 +299,7 @@ public function sharesDataProvider() { ] ], [ - // #4 bogus share: + // #6 bogus share: // - outsider shares with group1, group2 // - one subshare for each group share // - non-matching targets @@ -276,7 +324,7 @@ public function sharesDataProvider() { ] ], [ - // #5 bogus share: + // #7 bogus share: // - outsider shares with group1, group2 // - one subshare for each group share // - non-matching targets @@ -301,7 +349,7 @@ public function sharesDataProvider() { ] ], [ - // #6 bogus share: + // #8 bogus share: // - outsider shares with group1, group2 and also user2 // - one subshare for each group share // - one extra share entry for direct share to user2 @@ -329,7 +377,7 @@ public function sharesDataProvider() { ] ], [ - // #7 bogus share: + // #9 bogus share: // - outsider shares with group1 and also user2 // - no subshare at all // - one extra share entry for direct share to user2 @@ -350,7 +398,7 @@ public function sharesDataProvider() { ] ], [ - // #8 legitimate share with own group: + // #10 legitimate share with own group: // - insider shares with both groups the user is already in // - no subshares in this case [ @@ -368,7 +416,7 @@ public function sharesDataProvider() { ] ], [ - // #9 legitimate shares: + // #11 legitimate shares: // - group share with same group // - group share with other group // - user share where recipient renamed @@ -392,7 +440,7 @@ public function sharesDataProvider() { ] ], [ - // #10 legitimate share: + // #12 legitimate share: // - outsider shares with group and user directly with different permissions // - no subshares // - same targets From d2ad8041250d8fac4841a19cca69137474391c53 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 12 Aug 2016 12:06:57 +0200 Subject: [PATCH 12/12] Fix unmerged shares repair with mixed group and direct shares Whenever a group share is created after a direct share, the stime order needs to be properly considered in the repair routine, considering that the direct user share is appended to the $subShares array and breaking its order. --- lib/private/repair/repairunmergedshares.php | 16 +++++-- tests/lib/repair/repairunmergedsharestest.php | 45 ++++++++++++++++++- 2 files changed, 57 insertions(+), 4 deletions(-) diff --git a/lib/private/repair/repairunmergedshares.php b/lib/private/repair/repairunmergedshares.php index 723a153aee3e..b9ba5cf0ee52 100644 --- a/lib/private/repair/repairunmergedshares.php +++ b/lib/private/repair/repairunmergedshares.php @@ -93,7 +93,7 @@ private function buildPreparedQueries() { */ $query = $this->connection->getQueryBuilder(); $query - ->select('item_source', 'id', 'file_target', 'permissions', 'parent', 'share_type') + ->select('item_source', 'id', 'file_target', 'permissions', 'parent', 'share_type', 'stime') ->from('share') ->where($query->expr()->eq('share_type', $query->createParameter('shareType'))) ->andWhere($query->expr()->in('share_with', $query->createParameter('shareWiths'))) @@ -161,13 +161,23 @@ private function getSharesWithUser($shareType, $shareWiths) { * If no suitable subshare is found, use the least recent group share instead. * * @param array $groupShares group share entries - * @param array $subShares sub share entries, sorted by stime + * @param array $subShares sub share entries * * @return string chosen target name */ private function findBestTargetName($groupShares, $subShares) { $pickedShare = null; - // note subShares are sorted by stime from oldest to newest + // sort by stime, this also properly sorts the direct user share if any + @usort($subShares, function($a, $b) { + if ($a['stime'] < $b['stime']) { + return -1; + } else if ($a['stime'] > $b['stime']) { + return 1; + } + + return 0; + }); + foreach ($subShares as $subShare) { // skip entries that have parenthesis with numbers if (preg_match('/\([0-9]*\)/', $subShare['file_target']) === 1) { diff --git a/tests/lib/repair/repairunmergedsharestest.php b/tests/lib/repair/repairunmergedsharestest.php index 9fe9a0738810..7304bfd69208 100644 --- a/tests/lib/repair/repairunmergedsharestest.php +++ b/tests/lib/repair/repairunmergedsharestest.php @@ -44,6 +44,9 @@ class RepairUnmergedSharesTest extends TestCase { /** @var \OCP\IDBConnection */ private $connection; + /** @var int */ + private $lastShareTime; + protected function setUp() { parent::setUp(); @@ -92,6 +95,9 @@ protected function setUp() { } })); + // used to generate incremental stimes + $this->lastShareTime = time(); + /** @var \OCP\IConfig $config */ $this->repair = new RepairUnmergedShares($config, $this->connection, $userManager, $groupManager); } @@ -108,6 +114,7 @@ protected function deleteAllShares() { } private function createShare($type, $sourceId, $recipient, $targetName, $permissions, $parentId = null) { + $this->lastShareTime += 100; $qb = $this->connection->getQueryBuilder(); $values = [ 'share_type' => $qb->expr()->literal($type), @@ -119,7 +126,7 @@ private function createShare($type, $sourceId, $recipient, $targetName, $permiss 'file_source' => $qb->expr()->literal($sourceId), 'file_target' => $qb->expr()->literal($targetName), 'permissions' => $qb->expr()->literal($permissions), - 'stime' => $qb->expr()->literal(time()), + 'stime' => $qb->expr()->literal($this->lastShareTime), ]; if ($parentId !== null) { $values['parent'] = $qb->expr()->literal($parentId); @@ -458,6 +465,42 @@ public function sharesDataProvider() { ['/test (4)', 31], ] ], + [ + // #13 bogus share: + // - outsider shares with group1, user2 and then group2 + // - user renamed share as soon as it arrived before the next share (order) + // - one subshare for each group share + // - one extra share entry for direct share to user2 + // - non-matching targets + [ + // first share with group + [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup1', '/test', 31], + // recipient renames + [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/first', 31, 0], + // then direct share, user renames too + [Constants::SHARE_TYPE_USER, 123, 'user2', '/second', 31], + // another share with the second group + [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup2', '/test', 31], + // use renames it + [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/third', 31, 1], + // different unrelated share + [Constants::SHARE_TYPE_GROUP, 456, 'recipientgroup1', '/test (5)', 31], + ], + [ + // group share with group1 left alone + ['/test', 31], + // first subshare repaired + ['/third', 31], + // direct user share repaired + ['/third', 31], + // group share with group2 left alone + ['/test', 31], + // second subshare repaired + ['/third', 31], + // leave unrelated alone + ['/test (5)', 31], + ] + ], ]; }