diff --git a/apps/files_sharing/lib/MountProvider.php b/apps/files_sharing/lib/MountProvider.php index 1a08b5e9b65f2ba7e56bd5de9590cc07faf9c461..284728597cd216ef8c24169dc1d8d2a9686e07df 100644 --- a/apps/files_sharing/lib/MountProvider.php +++ b/apps/files_sharing/lib/MountProvider.php @@ -75,16 +75,20 @@ class MountProvider implements IMountProvider { return $share->getPermissions() > 0 && $share->getShareOwner() !== $user->getUID(); }); - $mounts = []; - foreach ($shares as $share) { + $superShares = $this->buildSuperShares($shares, $user); + $mounts = []; + foreach ($superShares as $share) { try { $mounts[] = new SharedMount( '\OC\Files\Storage\Shared', $mounts, [ 'user' => $user->getUID(), - 'newShare' => $share, + // parent share + 'superShare' => $share[0], + // children/component of the superShare + 'groupedShares' => $share[1], ], $storageFactory ); @@ -97,4 +101,84 @@ class MountProvider implements IMountProvider { // array_filter removes the null values from the array return array_filter($mounts); } + + /** + * Groups shares by path (nodeId) and target path + * + * @param \OCP\Share\IShare[] $shares + * @return \OCP\Share\IShare[][] array of grouped shares, each element in the + * array is a group which itself is an array of shares + */ + private function groupShares(array $shares) { + $tmp = []; + + foreach ($shares as $share) { + if (!isset($tmp[$share->getNodeId()])) { + $tmp[$share->getNodeId()] = []; + } + $tmp[$share->getNodeId()][] = $share; + } + + $result = []; + // sort by stime, the super share will be based on the least recent share + foreach ($tmp as &$tmp2) { + @usort($tmp2, function($a, $b) { + if ($a->getShareTime() < $b->getShareTime()) { + return -1; + } + return 1; + }); + $result[] = $tmp2; + } + + return array_values($result); + } + + /** + * Build super shares (virtual share) by grouping them by node id and target, + * then for each group compute the super share and return it along with the matching + * grouped shares. The most permissive permissions are used based on the permissions + * of all shares within the group. + * + * @param \OCP\Share\IShare[] $allShares + * @param \OCP\IUser $user user + * @return array Tuple of [superShare, groupedShares] + */ + private function buildSuperShares(array $allShares, \OCP\IUser $user) { + $result = []; + + $groupedShares = $this->groupShares($allShares); + + /** @var \OCP\Share\IShare[] $shares */ + foreach ($groupedShares as $shares) { + if (count($shares) === 0) { + continue; + } + + $superShare = $this->shareManager->newShare(); + + // compute super share based on first entry of the group + $superShare->setId($shares[0]->getId()) + ->setShareOwner($shares[0]->getShareOwner()) + ->setNodeId($shares[0]->getNodeId()) + ->setTarget($shares[0]->getTarget()); + + // use most permissive permissions + $permissions = 0; + foreach ($shares as $share) { + $permissions |= $share->getPermissions(); + if ($share->getTarget() !== $superShare->getTarget()) { + // adjust target, for database consistency + $share->setTarget($superShare->getTarget()); + $this->shareManager->moveShare($share, $user->getUID()); + } + } + + $superShare->setPermissions($permissions); + + $result[] = [$superShare, $shares]; + } + + return $result; + } } diff --git a/apps/files_sharing/lib/SharedMount.php b/apps/files_sharing/lib/SharedMount.php index e5b7edc8e0295a5cf6436716af5c3f10b0dc3f3a..57610db9076cbe4861fcb995707bc79635856361 100644 --- a/apps/files_sharing/lib/SharedMount.php +++ b/apps/files_sharing/lib/SharedMount.php @@ -52,7 +52,10 @@ class SharedMount extends MountPoint implements MoveableMount { private $user; /** @var \OCP\Share\IShare */ - private $share; + private $superShare; + + /** @var \OCP\Share\IShare[] */ + private $groupedShares; /** * @param string $storage @@ -63,10 +66,13 @@ class SharedMount extends MountPoint implements MoveableMount { public function __construct($storage, array $mountpoints, $arguments = null, $loader = null) { $this->user = $arguments['user']; $this->recipientView = new View('/' . $this->user . '/files'); - $this->share = $arguments['newShare']; - $newMountPoint = $this->verifyMountPoint($this->share, $mountpoints); + + $this->superShare = $arguments['superShare']; + $this->groupedShares = $arguments['groupedShares']; + + $newMountPoint = $this->verifyMountPoint($this->superShare, $mountpoints); $absMountPoint = '/' . $this->user . '/files' . $newMountPoint; - $arguments['ownerView'] = new View('/' . $this->share->getShareOwner() . '/files'); + $arguments['ownerView'] = new View('/' . $this->superShare->getShareOwner() . '/files'); parent::__construct($storage, $absMountPoint, $arguments, $loader); } @@ -108,7 +114,11 @@ class SharedMount extends MountPoint implements MoveableMount { */ private function updateFileTarget($newPath, &$share) { $share->setTarget($newPath); - \OC::$server->getShareManager()->moveShare($share, $this->user); + + foreach ($this->groupedShares as $share) { + $share->setTarget($newPath); + \OC::$server->getShareManager()->moveShare($share, $this->user); + } } @@ -214,7 +224,7 @@ class SharedMount extends MountPoint implements MoveableMount { * @return \OCP\Share\IShare */ public function getShare() { - return $this->share; + return $this->superShare; } /** @@ -223,6 +233,6 @@ class SharedMount extends MountPoint implements MoveableMount { * @return int */ public function getStorageRootId() { - return $this->share->getNodeId(); + return $this->getShare()->getNodeId(); } } diff --git a/apps/files_sharing/lib/sharedstorage.php b/apps/files_sharing/lib/sharedstorage.php index 710137cfe9ac165aa34ffa7ed2f343e0e9e12e33..8e9a0f41229bae9c2ef8bda8a0a6dd80f9998c07 100644 --- a/apps/files_sharing/lib/sharedstorage.php +++ b/apps/files_sharing/lib/sharedstorage.php @@ -43,11 +43,11 @@ use OCP\Lock\ILockingProvider; * Convert target path to source path and pass the function call to the correct storage provider */ class Shared extends \OC\Files\Storage\Wrapper\Jail implements ISharedStorage { - - private $share; // the shared resource - /** @var \OCP\Share\IShare */ - private $newShare; + private $superShare; + + /** @var \OCP\Share\IShare[] */ + private $groupedShares; /** * @var \OC\Files\View @@ -77,11 +77,14 @@ class Shared extends \OC\Files\Storage\Wrapper\Jail implements ISharedStorage { public function __construct($arguments) { $this->ownerView = $arguments['ownerView']; $this->logger = \OC::$server->getLogger(); - $this->newShare = $arguments['newShare']; + + $this->superShare = $arguments['superShare']; + $this->groupedShares = $arguments['groupedShares']; + $this->user = $arguments['user']; - Filesystem::initMountPoints($this->newShare->getShareOwner()); - $sourcePath = $this->ownerView->getPath($this->newShare->getNodeId()); + Filesystem::initMountPoints($this->superShare->getShareOwner()); + $sourcePath = $this->ownerView->getPath($this->superShare->getNodeId()); list($storage, $internalPath) = $this->ownerView->resolvePath($sourcePath); parent::__construct([ @@ -96,8 +99,8 @@ class Shared extends \OC\Files\Storage\Wrapper\Jail implements ISharedStorage { } $this->initialized = true; try { - Filesystem::initMountPoints($this->newShare->getShareOwner()); - $sourcePath = $this->ownerView->getPath($this->newShare->getNodeId()); + Filesystem::initMountPoints($this->superShare->getShareOwner()); + $sourcePath = $this->ownerView->getPath($this->superShare->getNodeId()); list($this->sourceStorage, $sourceInternalPath) = $this->ownerView->resolvePath($sourcePath); $this->sourceRootInfo = $this->sourceStorage->getCache()->get($sourceInternalPath); } catch (\Exception $e) { @@ -105,6 +108,13 @@ class Shared extends \OC\Files\Storage\Wrapper\Jail implements ISharedStorage { } } + /** + * @return string + */ + public function getShareId() { + return $this->superShare->getId(); + } + private function isValid() { $this->init(); return $this->sourceRootInfo && ($this->sourceRootInfo->getPermissions() & Constants::PERMISSION_SHARE) === Constants::PERMISSION_SHARE; @@ -119,15 +129,6 @@ class Shared extends \OC\Files\Storage\Wrapper\Jail implements ISharedStorage { return 'shared::' . $this->getMountPoint(); } - /** - * get file cache of the shared item source - * - * @return int - */ - public function getSourceId() { - return $this->newShare->getNodeId(); - } - /** * Get the permissions granted for a shared file * @@ -138,7 +139,7 @@ class Shared extends \OC\Files\Storage\Wrapper\Jail implements ISharedStorage { if (!$this->isValid()) { return 0; } - $permissions = $this->newShare->getPermissions(); + $permissions = $this->superShare->getPermissions(); // part files and the mount point always have delete permissions if ($target === '' || pathinfo($target, PATHINFO_EXTENSION) === 'part') { $permissions |= \OCP\Constants::PERMISSION_DELETE; @@ -260,30 +261,18 @@ class Shared extends \OC\Files\Storage\Wrapper\Jail implements ISharedStorage { * @return string */ public function getMountPoint() { - return $this->newShare->getTarget(); + return $this->superShare->getTarget(); } /** * @param string $path */ public function setMountPoint($path) { - $this->newShare->setTarget($path); - } - - /** - * @return int - */ - public function getShareType() { - return $this->newShare->getShareType(); - } + $this->superShare->setTarget($path); - /** - * get share ID - * - * @return integer unique share ID - */ - public function getShareId() { - return $this->newShare->getId(); + foreach ($this->groupedShares as $share) { + $share->setTarget($path); + } } /** @@ -292,14 +281,14 @@ class Shared extends \OC\Files\Storage\Wrapper\Jail implements ISharedStorage { * @return string */ public function getSharedFrom() { - return $this->newShare->getShareOwner(); + return $this->superShare->getShareOwner(); } /** * @return \OCP\Share\IShare */ public function getShare() { - return $this->newShare; + return $this->superShare; } /** @@ -308,7 +297,7 @@ class Shared extends \OC\Files\Storage\Wrapper\Jail implements ISharedStorage { * @return string */ public function getItemType() { - return $this->newShare->getNodeType(); + return $this->superShare->getNodeType(); } public function getCache($path = '', $storage = null) { @@ -337,7 +326,7 @@ class Shared extends \OC\Files\Storage\Wrapper\Jail implements ISharedStorage { } public function getOwner($path) { - return $this->newShare->getShareOwner(); + return $this->superShare->getShareOwner(); } /** @@ -346,7 +335,9 @@ class Shared extends \OC\Files\Storage\Wrapper\Jail implements ISharedStorage { * @return bool */ public function unshareStorage() { - \OC::$server->getShareManager()->deleteFromSelf($this->newShare, $this->user); + foreach ($this->groupedShares as $share) { + \OC::$server->getShareManager()->deleteFromSelf($share, $this->user); + } return true; } @@ -362,7 +353,7 @@ class Shared extends \OC\Files\Storage\Wrapper\Jail implements ISharedStorage { $targetStorage->acquireLock($targetInternalPath, $type, $provider); // lock the parent folders of the owner when locking the share as recipient if ($path === '') { - $sourcePath = $this->ownerView->getPath($this->newShare->getNodeId()); + $sourcePath = $this->ownerView->getPath($this->superShare->getNodeId()); $this->ownerView->lockFile(dirname($sourcePath), ILockingProvider::LOCK_SHARED, true); } } @@ -378,7 +369,7 @@ class Shared extends \OC\Files\Storage\Wrapper\Jail implements ISharedStorage { $targetStorage->releaseLock($targetInternalPath, $type, $provider); // unlock the parent folders of the owner when unlocking the share as recipient if ($path === '') { - $sourcePath = $this->ownerView->getPath($this->newShare->getNodeId()); + $sourcePath = $this->ownerView->getPath($this->superShare->getNodeId()); $this->ownerView->unlockFile(dirname($sourcePath), ILockingProvider::LOCK_SHARED, true); } } diff --git a/apps/files_sharing/tests/MountProviderTest.php b/apps/files_sharing/tests/MountProviderTest.php index 8fe43d454f657020f0f1f3afbeb015f1731391cc..576f05d565d0d502f83ec9bce45c13c2fa8accb8 100644 --- a/apps/files_sharing/tests/MountProviderTest.php +++ b/apps/files_sharing/tests/MountProviderTest.php @@ -68,51 +68,224 @@ class MountProviderTest extends \Test\TestCase { $this->provider = new MountProvider($this->config, $this->shareManager, $this->logger); } - public function testExcludeShares() { - /** @var IShare | \PHPUnit_Framework_MockObject_MockObject $share1 */ - $share1 = $this->getMockBuilder('\OCP\Share\IShare')->getMock(); - $share1->expects($this->once()) - ->method('getPermissions') - ->will($this->returnValue(0)); - - $share2 = $this->getMockBuilder('\OCP\Share\IShare')->getMock(); - $share2->expects($this->once()) + private function makeMockShare($id, $nodeId, $owner = 'user2', $target = null, $permissions = 31) { + $share = $this->getMock('\OCP\Share\IShare'); + $share->expects($this->any()) ->method('getPermissions') - ->will($this->returnValue(31)); - $share2->expects($this->any()) + ->will($this->returnValue($permissions)); + $share->expects($this->any()) ->method('getShareOwner') - ->will($this->returnValue('user2')); - $share2->expects($this->any()) + ->will($this->returnValue($owner)); + $share->expects($this->any()) ->method('getTarget') - ->will($this->returnValue('/share2')); + ->will($this->returnValue($target)); + $share->expects($this->any()) + ->method('getId') + ->will($this->returnValue($id)); + $share->expects($this->any()) + ->method('getNodeId') + ->will($this->returnValue($nodeId)); + $share->expects($this->any()) + ->method('getShareTime') + ->will($this->returnValue( + // compute share time based on id, simulating share order + new \DateTime('@' . (1469193980 + 1000 * $id)) + )); + return $share; + } - $share3 = $this->getMockBuilder('\OCP\Share\IShare')->getMock(); - $share3->expects($this->once()) - ->method('getPermissions') - ->will($this->returnValue(0)); + /** + * 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() { + $rootFolder = $this->getMock('\OCP\Files\IRootFolder'); + $userManager = $this->getMock('\OCP\IUserManager'); + $userShares = [ + $this->makeMockShare(1, 100, 'user2', '/share2', 0), + $this->makeMockShare(2, 100, 'user2', '/share2', 31), + ]; + $groupShares = [ + $this->makeMockShare(3, 100, 'user2', '/share2', 0), + $this->makeMockShare(4, 101, 'user2', '/share4', 31), + $this->makeMockShare(5, 100, 'user1', '/share4', 31), + ]; + $this->user->expects($this->any()) + ->method('getUID') + ->will($this->returnValue('user1')); + $this->shareManager->expects($this->at(0)) + ->method('getSharedWith') + ->with('user1', \OCP\Share::SHARE_TYPE_USER) + ->will($this->returnValue($userShares)); + $this->shareManager->expects($this->at(1)) + ->method('getSharedWith') + ->with('user1', \OCP\Share::SHARE_TYPE_GROUP, null, -1) + ->will($this->returnValue($groupShares)); + $this->shareManager->expects($this->any()) + ->method('newShare') + ->will($this->returnCallback(function() use ($rootFolder, $userManager) { + return new \OC\Share20\Share($rootFolder, $userManager); + })); + $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->getId()); + $this->assertEquals('user2', $mountedShare1->getShareOwner()); + $this->assertEquals(100, $mountedShare1->getNodeId()); + $this->assertEquals('/share2', $mountedShare1->getTarget()); + $this->assertEquals(31, $mountedShare1->getPermissions()); + $mountedShare2 = $mounts[1]->getShare(); + $this->assertEquals('4', $mountedShare2->getId()); + $this->assertEquals('user2', $mountedShare2->getShareOwner()); + $this->assertEquals(101, $mountedShare2->getNodeId()); + $this->assertEquals('/share4', $mountedShare2->getTarget()); + $this->assertEquals(31, $mountedShare2->getPermissions()); + } - /** @var IShare | \PHPUnit_Framework_MockObject_MockObject $share4 */ - $share4 = $this->getMockBuilder('\OCP\Share\IShare')->getMock(); - $share4->expects($this->once()) - ->method('getPermissions') - ->will($this->returnValue(31)); - $share4->expects($this->any()) - ->method('getShareOwner') - ->will($this->returnValue('user2')); - $share4->expects($this->any()) - ->method('getTarget') - ->will($this->returnValue('/share4')); + public function mergeSharesDataProvider() { + // note: the user in the specs here is the shareOwner not recipient + // the recipient is always "user1" + return [ + // #0: share as outsider with "group1" and "user1" with same permissions + [ + [ + [1, 100, 'user2', '/share2', 31], + ], + [ + [2, 100, 'user2', '/share2', 31], + ], + [ + // combined, user share has higher priority + ['1', 100, 'user2', '/share2', 31], + ], + ], + // #1: share as outsider with "group1" and "user1" with different permissions + [ + [ + [1, 100, 'user2', '/share', 31], + ], + [ + [2, 100, 'user2', '/share', 15], + ], + [ + // use highest permissions + ['1', 100, 'user2', '/share', 31], + ], + ], + // #2: share as outsider with "group1" and "group2" with same permissions + [ + [ + ], + [ + [1, 100, 'user2', '/share', 31], + [2, 100, 'user2', '/share', 31], + ], + [ + // combined, first group share has higher priority + ['1', 100, 'user2', '/share', 31], + ], + ], + // #3: share as outsider with "group1" and "group2" with different permissions + [ + [ + ], + [ + [1, 100, 'user2', '/share', 31], + [2, 100, 'user2', '/share', 15], + ], + [ + // use higher permissions + ['1', 100, 'user2', '/share', 31], + ], + ], + // #4: share as insider with "group1" + [ + [ + ], + [ + [1, 100, 'user1', '/share', 31], + ], + [ + // no received share since "user1" is the sharer/owner + ], + ], + // #5: share as insider with "group1" and "group2" with different permissions + [ + [ + ], + [ + [1, 100, 'user1', '/share', 31], + [2, 100, 'user1', '/share', 15], + ], + [ + // no received share since "user1" is the sharer/owner + ], + ], + // #6: share as outside with "group1", recipient opted out + [ + [ + ], + [ + [1, 100, 'user2', '/share', 0], + ], + [ + // no received share since "user1" opted out + ], + ], + // #7: share as outsider with "group1" and "user1" where recipient renamed in between + [ + [ + [1, 100, 'user2', '/share2-renamed', 31], + ], + [ + [2, 100, 'user2', '/share2', 31], + ], + [ + // use target of least recent share + ['1', 100, 'user2', '/share2-renamed', 31], + ], + ], + // #8: share as outsider with "group1" and "user1" where recipient renamed in between + [ + [ + [2, 100, 'user2', '/share2', 31], + ], + [ + [1, 100, 'user2', '/share2-renamed', 31], + ], + [ + // use target of least recent share + ['1', 100, 'user2', '/share2-renamed', 31], + ], + ], + ]; + } - $share5 = $this->getMockBuilder('\OCP\Share\IShare')->getMock(); - $share5->expects($this->once()) - ->method('getPermissions') - ->will($this->returnValue(31)); - $share5->expects($this->any()) - ->method('getShareOwner') - ->will($this->returnValue('user1')); + /** + * Tests merging shares. + * + * Happens when sharing the same entry to a user through multiple ways, + * like several groups and also direct shares at the same time. + * + * @dataProvider mergeSharesDataProvider + * + * @param array $userShares array of user share specs + * @param array $groupShares array of group share specs + * @param array $expectedShares array of expected supershare specs + */ + public function testMergeShares($userShares, $groupShares, $expectedShares) { + $rootFolder = $this->getMock('\OCP\Files\IRootFolder'); + $userManager = $this->getMock('\OCP\IUserManager'); - $userShares = [$share1, $share2]; - $groupShares = [$share3, $share4, $share5]; + $userShares = array_map(function($shareSpec) { + return $this->makeMockShare($shareSpec[0], $shareSpec[1], $shareSpec[2], $shareSpec[3], $shareSpec[4]); + }, $userShares); + $groupShares = array_map(function($shareSpec) { + return $this->makeMockShare($shareSpec[0], $shareSpec[1], $shareSpec[2], $shareSpec[3], $shareSpec[4]); + }, $groupShares); $this->user->expects($this->any()) ->method('getUID') @@ -126,17 +299,29 @@ class MountProviderTest extends \Test\TestCase { ->method('getSharedWith') ->with('user1', \OCP\Share::SHARE_TYPE_GROUP, null, -1) ->will($this->returnValue($groupShares)); + $this->shareManager->expects($this->any()) + ->method('newShare') + ->will($this->returnCallback(function() use ($rootFolder, $userManager) { + return new \OC\Share20\Share($rootFolder, $userManager); + })); $mounts = $this->provider->getMountsForUser($this->user, $this->loader); - $this->assertCount(2, $mounts); - $this->assertSharedMount($share1, $mounts[0]); - $this->assertSharedMount($share4, $mounts[1]); - } + $this->assertCount(count($expectedShares), $mounts); + + foreach ($mounts as $index => $mount) { + $expectedShare = $expectedShares[$index]; + $this->assertInstanceOf('OCA\Files_Sharing\SharedMount', $mount); + + // supershare + $share = $mount->getShare(); - private function assertSharedMount(IShare $share, IMountPoint $mount) { - $this->assertInstanceOf('OCA\Files_Sharing\SharedMount', $mount); - $this->assertEquals($share, $mount->getShare()); + $this->assertEquals($expectedShare[0], $share->getId()); + $this->assertEquals($expectedShare[1], $share->getNodeId()); + $this->assertEquals($expectedShare[2], $share->getShareOwner()); + $this->assertEquals($expectedShare[3], $share->getTarget()); + $this->assertEquals($expectedShare[4], $share->getPermissions()); + } } } diff --git a/build/integration/features/bootstrap/Sharing.php b/build/integration/features/bootstrap/Sharing.php index 9d230e35ac6994f86e8dd19943ee9f425874e74e..954c273bfc2fe96a2b8cb4bd711aa358f741ff21 100644 --- a/build/integration/features/bootstrap/Sharing.php +++ b/build/integration/features/bootstrap/Sharing.php @@ -309,10 +309,10 @@ trait Sharing { PHPUnit_Framework_Assert::assertEquals(False, $this->isFieldInResponse('share_with', "$user")); } - public function isUserOrGroupInSharedData($userOrGroup){ + public function isUserOrGroupInSharedData($userOrGroup, $permissions = null){ $data = $this->response->xml()->data[0]; foreach($data as $element) { - if ($element->share_with == $userOrGroup){ + if ($element->share_with == $userOrGroup && ($permissions === null || $permissions == $element->permissions)){ return True; } } @@ -320,13 +320,13 @@ trait Sharing { } /** - * @Given /^file "([^"]*)" of user "([^"]*)" is shared with user "([^"]*)"$/ + * @Given /^(file|folder|entry) "([^"]*)" of user "([^"]*)" is shared with user "([^"]*)"( with permissions ([\d]*))?$/ * * @param string $filepath * @param string $user1 * @param string $user2 */ - public function assureFileIsShared($filepath, $user1, $user2){ + public function assureFileIsShared($entry, $filepath, $user1, $user2, $withPerms = null, $permissions = null){ $fullUrl = $this->baseUrl . "v{$this->apiVersion}.php/apps/files_sharing/api/v{$this->sharingApiVersion}/shares" . "?path=$filepath"; $client = new Client(); $options = []; @@ -336,23 +336,23 @@ trait Sharing { $options['auth'] = [$user1, $this->regularUser]; } $this->response = $client->get($fullUrl, $options); - if ($this->isUserOrGroupInSharedData($user2)){ + if ($this->isUserOrGroupInSharedData($user2, $permissions)){ return; } else { - $this->createShare($user1, $filepath, 0, $user2, null, null, null); + $this->createShare($user1, $filepath, 0, $user2, null, null, $permissions); } $this->response = $client->get($fullUrl, $options); - PHPUnit_Framework_Assert::assertEquals(True, $this->isUserOrGroupInSharedData($user2)); + PHPUnit_Framework_Assert::assertEquals(True, $this->isUserOrGroupInSharedData($user2, $permissions)); } /** - * @Given /^file "([^"]*)" of user "([^"]*)" is shared with group "([^"]*)"$/ + * @Given /^(file|folder|entry) "([^"]*)" of user "([^"]*)" is shared with group "([^"]*)"( with permissions ([\d]*))?$/ * * @param string $filepath * @param string $user * @param string $group */ - public function assureFileIsSharedWithGroup($filepath, $user, $group){ + public function assureFileIsSharedWithGroup($entry, $filepath, $user, $group, $withPerms = null, $permissions = null){ $fullUrl = $this->baseUrl . "v{$this->apiVersion}.php/apps/files_sharing/api/v{$this->sharingApiVersion}/shares" . "?path=$filepath"; $client = new Client(); $options = []; @@ -362,13 +362,13 @@ trait Sharing { $options['auth'] = [$user, $this->regularUser]; } $this->response = $client->get($fullUrl, $options); - if ($this->isUserOrGroupInSharedData($group)){ + if ($this->isUserOrGroupInSharedData($group, $permissions)){ return; } else { - $this->createShare($user, $filepath, 1, $group, null, null, null); + $this->createShare($user, $filepath, 1, $group, null, null, $permissions); } $this->response = $client->get($fullUrl, $options); - PHPUnit_Framework_Assert::assertEquals(True, $this->isUserOrGroupInSharedData($group)); + PHPUnit_Framework_Assert::assertEquals(True, $this->isUserOrGroupInSharedData($group, $permissions)); } /** diff --git a/build/integration/features/bootstrap/WebDav.php b/build/integration/features/bootstrap/WebDav.php index 71f938c7ec4e5dac5500b77dc39af66d4b0d55c2..02f3e82a4c34edc5a2288ff3f84539fc197763d8 100644 --- a/build/integration/features/bootstrap/WebDav.php +++ b/build/integration/features/bootstrap/WebDav.php @@ -87,12 +87,12 @@ trait WebDav { } /** - * @Given /^User "([^"]*)" moved file "([^"]*)" to "([^"]*)"$/ + * @Given /^User "([^"]*)" moved (file|folder|entry) "([^"]*)" to "([^"]*)"$/ * @param string $user * @param string $fileSource * @param string $fileDestination */ - public function userMovedFile($user, $fileSource, $fileDestination){ + public function userMovedFile($user, $entry, $fileSource, $fileDestination){ $fullUrl = substr($this->baseUrl, 0, -4) . $this->davPath; $headers['Destination'] = $fullUrl . $fileDestination; $this->response = $this->makeDavRequest($user, "MOVE", $fileSource, $headers); @@ -100,12 +100,12 @@ trait WebDav { } /** - * @When /^User "([^"]*)" moves file "([^"]*)" to "([^"]*)"$/ + * @When /^User "([^"]*)" moves (file|folder|entry) "([^"]*)" to "([^"]*)"$/ * @param string $user * @param string $fileSource * @param string $fileDestination */ - public function userMovesFile($user, $fileSource, $fileDestination){ + public function userMovesFile($user, $entry, $fileSource, $fileDestination){ $fullUrl = substr($this->baseUrl, 0, -4) . $this->davPath; $headers['Destination'] = $fullUrl . $fileDestination; $this->response = $this->makeDavRequest($user, "MOVE", $fileSource, $headers); @@ -259,6 +259,32 @@ trait WebDav { $this->response = $this->listFolder($user, $path, 0, $properties); } + /** + * @Then /^as "([^"]*)" the (file|folder|entry) "([^"]*)" does not exist$/ + * @param string $user + * @param string $path + * @param \Behat\Gherkin\Node\TableNode|null $propertiesTable + */ + public function asTheFileOrFolderDoesNotExist($user, $entry, $path) { + $client = $this->getSabreClient($user); + $response = $client->request('HEAD', $this->makeSabrePath($path)); + if ($response['statusCode'] !== 404) { + throw new \Exception($entry . ' "' . $path . '" expected to not exist (status code ' . $response['statusCode'] . ', expected 404)'); + } + + return $response; + } + + /** + * @Then /^as "([^"]*)" the (file|folder|entry) "([^"]*)" exists$/ + * @param string $user + * @param string $path + * @param \Behat\Gherkin\Node\TableNode|null $propertiesTable + */ + public function asTheFileOrFolderExists($user, $entry, $path) { + $this->response = $this->listFolder($user, $path, 0); + } + /** * @Then the single response should contain a property :key with value :value * @param string $key @@ -327,9 +353,25 @@ trait WebDav { } } - /*Returns the elements of a propfind, $folderDepth requires 1 to see elements without children*/ public function listFolder($user, $path, $folderDepth, $properties = null){ + $client = $this->getSabreClient($user); + if (!$properties) { + $properties = [ + '{DAV:}getetag' + ]; + } + + $response = $client->propfind($this->makeSabrePath($path), $properties, $folderDepth); + + return $response; + } + + public function makeSabrePath($path) { + return $this->encodePath($this->davPath . '/' . ltrim($path, '/')); + } + + public function getSabreClient($user) { $fullUrl = substr($this->baseUrl, 0, -4); $settings = array( @@ -343,17 +385,7 @@ trait WebDav { $settings['password'] = $this->regularUser; } - $client = new SClient($settings); - - if (!$properties) { - $properties = [ - '{DAV:}getetag' - ]; - } - - $response = $client->propfind($this->davPath . '/' . ltrim($path, '/'), $properties, $folderDepth); - - return $response; + return new SClient($settings); } /** @@ -493,6 +525,17 @@ trait WebDav { } } + /** + * URL encodes the given path but keeps the slashes + * + * @param string $path to encode + * @return string encoded path + */ + private function encodePath($path) { + // slashes need to stay + return str_replace('%2F', '/', rawurlencode($path)); + } + /** * @When user :user favorites element :path */ diff --git a/build/integration/features/sharing-v1.feature b/build/integration/features/sharing-v1.feature index 94d12ce3e72eb36379dc6ca6b20d35b7bf8caa79..016b204bf90ae50c7aac5ed8fb99d2a28256e452 100644 --- a/build/integration/features/sharing-v1.feature +++ b/build/integration/features/sharing-v1.feature @@ -775,3 +775,146 @@ Feature: sharing And Deleting last share Then the OCS status code should be "404" And the HTTP status code should be "200" + + 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 + + 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 diff --git a/core/js/shareitemmodel.js b/core/js/shareitemmodel.js index 8ea0bf28b388a7c2cd922909852c1dc6fa38c157..00c1212fa79c4d8d1ba1fac8933129b1a9345b76 100644 --- a/core/js/shareitemmodel.js +++ b/core/js/shareitemmodel.js @@ -598,6 +598,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); @@ -615,7 +642,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 8c9560d2646d64535228dc7e145563a5f5cf7162..9d9001dc9e8f630a10929b5628aba01fb047d8b5 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([])); diff --git a/lib/private/Repair.php b/lib/private/Repair.php index 6c1eef5b9f66fcd59caf6349ebf61f02ce8e477f..cd23f5cb806edd7136f93a83e737e49dd5fb9492 100644 --- a/lib/private/Repair.php +++ b/lib/private/Repair.php @@ -49,6 +49,7 @@ use OC\Repair\RepairMimeTypes; use OC\Repair\SearchLuceneTables; use OC\Repair\UpdateOutdatedOcsIds; use OC\Repair\RepairInvalidShares; +use OC\Repair\RepairUnmergedShares; use OCP\AppFramework\QueryException; use OCP\Migration\IOutput; use OCP\Migration\IRepairStep; @@ -140,6 +141,12 @@ class Repair implements IOutput{ new RemoveOldShares(\OC::$server->getDatabaseConnection()), new AvatarPermissions(\OC::$server->getDatabaseConnection()), new RemoveRootShares(\OC::$server->getDatabaseConnection(), \OC::$server->getUserManager(), \OC::$server->getLazyRootFolder()), + new RepairUnmergedShares( + \OC::$server->getConfig(), + \OC::$server->getDatabaseConnection(), + \OC::$server->getUserManager(), + \OC::$server->getGroupManager() + ), ]; } diff --git a/lib/private/Repair/RepairUnmergedShares.php b/lib/private/Repair/RepairUnmergedShares.php new file mode 100644 index 0000000000000000000000000000000000000000..353877bb8737d49c99e3da218da236b6b0cfb093 --- /dev/null +++ b/lib/private/Repair/RepairUnmergedShares.php @@ -0,0 +1,328 @@ +<?php +/** + * @author Vincent Petry <pvince81@owncloud.com> + * + * @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 <http://www.gnu.org/licenses/> + * + */ + +namespace OC\Repair; + +use OCP\Migration\IOutput; +use OCP\Migration\IRepairStep; +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 implements IRepairStep { + + /** @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) { + if (empty($subShares)) { + return false; + } + + $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(IOutput $out, IUser $user) { + $groups = $this->groupManager->getUserGroupIds($user); + if (empty($groups)) { + // user is in no groups, so can't have received group shares + return; + } + + // get all subshares grouped by item source + $subSharesByItemSource = $this->getSharesWithUser(DefaultShareProvider::SHARE_TYPE_USERGROUP, [$user->getUID()]); + + // 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) && empty($userSharesByItemSource)) { + // nothing to repair for this user + return; + } + + foreach ($groupSharesByItemSource as $itemSource => $groupShares) { + $subShares = []; + if (isset($subSharesByItemSource[$itemSource])) { + $subShares = $subSharesByItemSource[$itemSource]; + } + + if (isset($userSharesByItemSource[$itemSource])) { + // add it to the subshares to get a similar treatment + $subShares = array_merge($subShares, $userSharesByItemSource[$itemSource]); + } + + $this->fixThisShare($groupShares, $subShares); + } + } + + /** + * 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(IOutput $output) { + $ocVersionFromBeforeUpdate = $this->config->getSystemValue('version', '0.0.0'); + if (version_compare($ocVersionFromBeforeUpdate, '9.1.0.16', '<')) { + // this situation was only possible between 9.0.0 and 9.0.3 included + + $function = function(IUser $user) use ($output) { + $this->fixUnmergedShares($output, $user); + $output->advance(); + }; + + $this->buildPreparedQueries(); + + $userCount = $this->countUsers(); + $output->startProgress($userCount); + + $this->userManager->callForAllUsers($function); + + $output->finishProgress(); + } + } +} diff --git a/tests/lib/Repair/RepairUnmergedSharesTest.php b/tests/lib/Repair/RepairUnmergedSharesTest.php new file mode 100644 index 0000000000000000000000000000000000000000..fe9b3e5b96f37ce2ce52e947b6fd951162092919 --- /dev/null +++ b/tests/lib/Repair/RepairUnmergedSharesTest.php @@ -0,0 +1,449 @@ +<?php +/** + * @author Vincent Petry <pvince81@owncloud.com> + * + * @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 <http://www.gnu.org/licenses/> + * + */ + +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 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 + [ + [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], + ] + ], + [ + // #9 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], + ] + ], + [ + // #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], + ] + ], + ]; + } + + /** + * 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']); + } + } +} + diff --git a/version.php b/version.php index 562dda0a792e5f88321ad4a6ce7941331bbd2719..e6298ae238f429c7f469fd91910fd3c8145347e1 100644 --- a/version.php +++ b/version.php @@ -25,7 +25,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, 2, 0, 0); +$OC_Version = array(9, 2, 0, 1); // The human readable string $OC_VersionString = '11.0 alpha';