From 0f5da29501f30adca28cbafd5f3568d2dbdebf74 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 22 Jul 2016 11:42:21 +0200 Subject: [PATCH 1/2] Add integration tests for double shares with rename in between --- .../integration/features/bootstrap/WebDav.php | 8 ++--- build/integration/features/sharing-v1.feature | 30 +++++++++++++++++++ 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/build/integration/features/bootstrap/WebDav.php b/build/integration/features/bootstrap/WebDav.php index e9e8c6233856..092c79fe0206 100644 --- a/build/integration/features/bootstrap/WebDav.php +++ b/build/integration/features/bootstrap/WebDav.php @@ -46,12 +46,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); @@ -59,12 +59,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; try { diff --git a/build/integration/features/sharing-v1.feature b/build/integration/features/sharing-v1.feature index a1b0428c05ef..0faf5f516d3e 100644 --- a/build/integration/features/sharing-v1.feature +++ b/build/integration/features/sharing-v1.feature @@ -838,3 +838,33 @@ Feature: sharing And as "user0" the folder "merge-test-inside-twogroups-perms (2)" does not exist And as "user0" the folder "merge-test-inside-twogroups-perms (3)" does not exist + Scenario: Merging shares for recipient when shared from outside with group then user and recipient renames in between + Given As an "admin" + And user "user0" exists + And user "user1" exists + And group "group1" exists + And user "user1" belongs to group "group1" + And user "user0" created a folder "merge-test-outside-groups-renamebeforesecondshare" + When folder "merge-test-outside-groups-renamebeforesecondshare" of user "user0" is shared with group "group1" + And User "user1" moved folder "/merge-test-outside-groups-renamebeforesecondshare" to "/merge-test-outside-groups-renamebeforesecondshare-renamed" + And folder "merge-test-outside-groups-renamebeforesecondshare" of user "user0" is shared with user "user1" + Then as "user1" gets properties of folder "merge-test-outside-groups-renamebeforesecondshare-renamed" with + |{http://owncloud.org/ns}permissions| + And the single response should contain a property "{http://owncloud.org/ns}permissions" with value "SRDNVCK" + And as "user1" the folder "merge-test-outside-groups-renamebeforesecondshare" does not exist + + Scenario: Merging shares for recipient when shared from outside with user then group and recipient renames in between + Given As an "admin" + And user "user0" exists + And user "user1" exists + And group "group1" exists + And user "user1" belongs to group "group1" + And user "user0" created a folder "merge-test-outside-groups-renamebeforesecondshare" + When folder "merge-test-outside-groups-renamebeforesecondshare" of user "user0" is shared with user "user1" + And User "user1" moved folder "/merge-test-outside-groups-renamebeforesecondshare" to "/merge-test-outside-groups-renamebeforesecondshare-renamed" + And folder "merge-test-outside-groups-renamebeforesecondshare" of user "user0" is shared with group "group1" + Then as "user1" gets properties of folder "merge-test-outside-groups-renamebeforesecondshare-renamed" with + |{http://owncloud.org/ns}permissions| + And the single response should contain a property "{http://owncloud.org/ns}permissions" with value "SRDNVCK" + And as "user1" the folder "merge-test-outside-groups-renamebeforesecondshare" does not exist + From 1a9a9e7da73f7460d9864db244d8bf967f1220bd Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 22 Jul 2016 15:30:13 +0200 Subject: [PATCH 2/2] Make share target consistent when grouping group share with user share In some situations, a group share is created before a user share, and the recipient renamed the received share before the latter is created. In this situation, the "file_target" was already modified and the second created share must align to the already renamed share. To achieve this, the MountProvider now groups only by "item_source" value and sorts by share time. This makes it so that the least recent share is selected as super-share and its "file_target" value is then adjusted in all grouped shares. This fixes the issue where this situation would have different "file_target" values resulting in two shared folders appearing instead of one. --- apps/files_sharing/lib/MountProvider.php | 27 +++++++++----- .../files_sharing/tests/MountProviderTest.php | 36 +++++++++++++++++-- 2 files changed, 53 insertions(+), 10 deletions(-) diff --git a/apps/files_sharing/lib/MountProvider.php b/apps/files_sharing/lib/MountProvider.php index c2e08e3a4a49..36a4d5adde9c 100644 --- a/apps/files_sharing/lib/MountProvider.php +++ b/apps/files_sharing/lib/MountProvider.php @@ -74,7 +74,7 @@ public function getMountsForUser(IUser $user, IStorageFactory $storageFactory) { return $share->getPermissions() > 0 && $share->getShareOwner() !== $user->getUID(); }); - $superShares = $this->buildSuperShares($shares); + $superShares = $this->buildSuperShares($shares, $user); $mounts = []; foreach ($superShares as $share) { @@ -115,17 +115,22 @@ private function groupShares(array $shares) { if (!isset($tmp[$share->getNodeId()])) { $tmp[$share->getNodeId()] = []; } - $tmp[$share->getNodeId()][$share->getTarget()][] = $share; + $tmp[$share->getNodeId()][] = $share; } $result = []; - foreach ($tmp as $tmp2) { - foreach ($tmp2 as $item) { - $result[] = $item; - } + // 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 $result; + return array_values($result); } /** @@ -135,9 +140,10 @@ private function groupShares(array $shares) { * 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) { + private function buildSuperShares(array $allShares, \OCP\IUser $user) { $result = []; $groupedShares = $this->groupShares($allShares); @@ -160,6 +166,11 @@ private function buildSuperShares(array $allShares) { $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); diff --git a/apps/files_sharing/tests/MountProviderTest.php b/apps/files_sharing/tests/MountProviderTest.php index 950a46a27f58..9f3d0450d85d 100644 --- a/apps/files_sharing/tests/MountProviderTest.php +++ b/apps/files_sharing/tests/MountProviderTest.php @@ -83,6 +83,12 @@ private function makeMockShare($id, $nodeId, $owner = 'user2', $target = null, $ $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; } @@ -102,7 +108,7 @@ public function testExcludeShares() { $groupShares = [ $this->makeMockShare(3, 100, 'user2', '/share2', 0), - $this->makeMockShare(4, 100, 'user2', '/share4', 31), + $this->makeMockShare(4, 101, 'user2', '/share4', 31), $this->makeMockShare(5, 100, 'user1', '/share4', 31), ]; @@ -141,7 +147,7 @@ public function testExcludeShares() { $mountedShare2 = $mounts[1]->getShare(); $this->assertEquals('4', $mountedShare2->getId()); $this->assertEquals('user2', $mountedShare2->getShareOwner()); - $this->assertEquals(100, $mountedShare2->getNodeId()); + $this->assertEquals(101, $mountedShare2->getNodeId()); $this->assertEquals('/share4', $mountedShare2->getTarget()); $this->assertEquals(31, $mountedShare2->getPermissions()); } @@ -236,6 +242,32 @@ public function mergeSharesDataProvider() { // 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], + ], + ], ]; }