diff --git a/apps/files_sharing/lib/MountProvider.php b/apps/files_sharing/lib/MountProvider.php
index 1a08b5e9b65f2..284728597cd21 100644
--- a/apps/files_sharing/lib/MountProvider.php
+++ b/apps/files_sharing/lib/MountProvider.php
@@ -75,16 +75,20 @@ public function getMountsForUser(IUser $user, IStorageFactory $storageFactory) {
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 @@ public function getMountsForUser(IUser $user, IStorageFactory $storageFactory) {
// 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 e5b7edc8e0295..57610db9076cb 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 @@ private function verifyMountPoint(\OCP\Share\IShare $share, array $mountpoints)
*/
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 @@ public function removeMount() {
* @return \OCP\Share\IShare
*/
public function getShare() {
- return $this->share;
+ return $this->superShare;
}
/**
@@ -223,6 +233,6 @@ public function getShare() {
* @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 710137cfe9ac1..8e9a0f41229ba 100644
--- a/apps/files_sharing/lib/sharedstorage.php
+++ b/apps/files_sharing/lib/sharedstorage.php
@@ -43,11 +43,11 @@
* 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 @@ private function init() {
}
$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 @@ private function init() {
}
}
+ /**
+ * @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 @@ public function getId() {
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 @@ public function getPermissions($target = '') {
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 @@ public function rename($path1, $path2) {
* @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 @@ public function getShareId() {
* @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 @@ public function getShare() {
* @return string
*/
public function getItemType() {
- return $this->newShare->getNodeType();
+ return $this->superShare->getNodeType();
}
public function getCache($path = '', $storage = null) {
@@ -337,7 +326,7 @@ public function getPropagator($storage = null) {
}
public function getOwner($path) {
- return $this->newShare->getShareOwner();
+ return $this->superShare->getShareOwner();
}
/**
@@ -346,7 +335,9 @@ public function getOwner($path) {
* @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 @@ public function acquireLock($path, $type, ILockingProvider $provider) {
$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 @@ public function releaseLock($path, $type, ILockingProvider $provider) {
$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 8fe43d454f657..576f05d565d0d 100644
--- a/apps/files_sharing/tests/MountProviderTest.php
+++ b/apps/files_sharing/tests/MountProviderTest.php
@@ -68,51 +68,224 @@ public function setUp() {
$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 @@ public function testExcludeShares() {
->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 9d230e35ac699..954c273bfc2fe 100644
--- a/build/integration/features/bootstrap/Sharing.php
+++ b/build/integration/features/bootstrap/Sharing.php
@@ -309,10 +309,10 @@ public function checkSharedUserNotInResponse($user){
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 @@ public function isUserOrGroupInSharedData($userOrGroup){
}
/**
- * @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 @@ public function assureFileIsShared($filepath, $user1, $user2){
$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 @@ public function assureFileIsSharedWithGroup($filepath, $user, $group){
$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 71f938c7ec4e5..02f3e82a4c34e 100644
--- a/build/integration/features/bootstrap/WebDav.php
+++ b/build/integration/features/bootstrap/WebDav.php
@@ -87,12 +87,12 @@ public function makeDavRequest($user, $method, $path, $headers, $body = null){
}
/**
- * @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 @@ public function userMovedFile($user, $fileSource, $fileDestination){
}
/**
- * @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 @@ public function asGetsPropertiesOfFolderWith($user, $path, $propertiesTable) {
$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 @@ public function theResponseShouldContainAnEmptyProperty($property) {
}
}
-
/*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 @@ public function listFolder($user, $path, $folderDepth, $properties = null){
$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 @@ public function downloadingFileAs($fileName, $user) {
}
}
+ /**
+ * 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 94d12ce3e72eb..016b204bf90ae 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 8ea0bf28b388a..00c1212fa79c4 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 8c9560d2646d6..9d9001dc9e8f6 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 6c1eef5b9f66f..cd23f5cb806ed 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;
use OCP\AppFramework\QueryException;
use OCP\Migration\IOutput;
use OCP\Migration\IRepairStep;
@@ -140,6 +141,12 @@ public static function getRepairSteps() {
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 0000000000000..353877bb8737d
--- /dev/null
+++ b/lib/private/Repair/RepairUnmergedShares.php
@@ -0,0 +1,328 @@
+
+ *
+ * @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 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 0000000000000..fe9b3e5b96f37
--- /dev/null
+++ b/tests/lib/Repair/RepairUnmergedSharesTest.php
@@ -0,0 +1,449 @@
+
+ *
+ * @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 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 562dda0a792e5..e6298ae238f42 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';