From 56b94b220da4d947438ffee7a4e889ef7f61bbde Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 12 Aug 2016 11:44:34 +0200 Subject: [PATCH 1/3] Improve file_target finding logic when repairing unmerged shares Pick the most recent subshare that has no parenthesis from duplication which should match whichever name the user picked last. If all subshares have duplicate parenthesis names, use the least recent group share's target instead. --- lib/private/Repair/RepairUnmergedShares.php | 40 ++++++++++- tests/lib/Repair/RepairUnmergedSharesTest.php | 66 ++++++++++++++++--- 2 files changed, 96 insertions(+), 10 deletions(-) diff --git a/lib/private/Repair/RepairUnmergedShares.php b/lib/private/Repair/RepairUnmergedShares.php index 353877bb8737d..f14e98c17619a 100644 --- a/lib/private/Repair/RepairUnmergedShares.php +++ b/lib/private/Repair/RepairUnmergedShares.php @@ -148,6 +148,44 @@ private function getSharesWithUser($shareType, $shareWiths) { return $groupedShares; } + /** + * Decide on the best target name based on all group shares and subshares, + * goal is to increase the likeliness that the chosen name matches what + * the user is expecting. + * + * For this, we discard the entries with parenthesis "(2)". + * In case the user also renamed the duplicates to a legitimate name, this logic + * will still pick the most recent one as it's the one the user is most likely to + * remember renaming. + * + * If no suitable subshare is found, use the least recent group share instead. + * + * @param array $groupShares group share entries + * @param array $subShares sub share entries, sorted by stime + * + * @return string chosen target name + */ + private function findBestTargetName($groupShares, $subShares) { + $pickedShare = null; + // note subShares are sorted by stime from oldest to newest + foreach ($subShares as $subShare) { + // skip entries that have parenthesis with numbers + if (preg_match('/\([0-9]*\)/', $subShare['file_target']) === 1) { + continue; + } + // pick any share found that would match, the last being the most recent + $pickedShare = $subShare; + } + + // no suitable subshare found + if ($pickedShare === null) { + // use least recent group share target instead + $pickedShare = $groupShares[0]; + } + + return $pickedShare['file_target']; + } + /** * Fix the given received share represented by the set of group shares * and matching sub shares @@ -171,7 +209,7 @@ private function fixThisShare($groupShares, $subShares) { return false; } - $targetPath = $groupShares[0]['file_target']; + $targetPath = $this->findBestTargetName($groupShares, $subShares); // check whether the user opted out completely of all subshares $optedOut = true; diff --git a/tests/lib/Repair/RepairUnmergedSharesTest.php b/tests/lib/Repair/RepairUnmergedSharesTest.php index fe9b3e5b96f37..9fe9a07388102 100644 --- a/tests/lib/Repair/RepairUnmergedSharesTest.php +++ b/tests/lib/Repair/RepairUnmergedSharesTest.php @@ -204,7 +204,7 @@ public function sharesDataProvider() { [ // #2 bogus share // - outsider shares with group1, group2 - // - one subshare for each group share + // - one subshare for each group share, both with parenthesis // - but the targets do not match when grouped [ [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup1', '/test', 31], @@ -218,7 +218,7 @@ public function sharesDataProvider() { [ ['/test', 31], ['/test', 31], - // reset to original name + // reset to original name as the sub-names have parenthesis ['/test', 31], ['/test', 31], // leave unrelated alone @@ -228,6 +228,54 @@ public function sharesDataProvider() { [ // #3 bogus share // - outsider shares with group1, group2 + // - one subshare for each group share, both renamed manually + // - but the targets do not match when grouped + [ + [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup1', '/test', 31], + [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup2', '/test', 31], + // child of the previous ones + [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test_renamed (1 legit paren)', 31, 0], + [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test_renamed (2 legit paren)', 31, 1], + // different unrelated share + [Constants::SHARE_TYPE_GROUP, 456, 'recipientgroup1', '/test (4)', 31], + ], + [ + ['/test', 31], + ['/test', 31], + // reset to less recent subshare name + ['/test_renamed (2 legit paren)', 31], + ['/test_renamed (2 legit paren)', 31], + // leave unrelated alone + ['/test (4)', 31], + ] + ], + [ + // #4 bogus share + // - outsider shares with group1, group2 + // - one subshare for each group share, one with parenthesis + // - but the targets do not match when grouped + [ + [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup1', '/test', 31], + [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup2', '/test', 31], + // child of the previous ones + [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test (2)', 31, 0], + [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test_renamed', 31, 1], + // different unrelated share + [Constants::SHARE_TYPE_GROUP, 456, 'recipientgroup1', '/test (4)', 31], + ], + [ + ['/test', 31], + ['/test', 31], + // reset to less recent subshare name but without parenthesis + ['/test_renamed', 31], + ['/test_renamed', 31], + // leave unrelated alone + ['/test (4)', 31], + ] + ], + [ + // #5 bogus share + // - outsider shares with group1, group2 // - one subshare for each group share // - first subshare not renamed (as in real world scenario) // - but the targets do not match when grouped @@ -251,7 +299,7 @@ public function sharesDataProvider() { ] ], [ - // #4 bogus share: + // #6 bogus share: // - outsider shares with group1, group2 // - one subshare for each group share // - non-matching targets @@ -276,7 +324,7 @@ public function sharesDataProvider() { ] ], [ - // #5 bogus share: + // #7 bogus share: // - outsider shares with group1, group2 // - one subshare for each group share // - non-matching targets @@ -301,7 +349,7 @@ public function sharesDataProvider() { ] ], [ - // #6 bogus share: + // #8 bogus share: // - outsider shares with group1, group2 and also user2 // - one subshare for each group share // - one extra share entry for direct share to user2 @@ -329,7 +377,7 @@ public function sharesDataProvider() { ] ], [ - // #7 bogus share: + // #9 bogus share: // - outsider shares with group1 and also user2 // - no subshare at all // - one extra share entry for direct share to user2 @@ -350,7 +398,7 @@ public function sharesDataProvider() { ] ], [ - // #8 legitimate share with own group: + // #10 legitimate share with own group: // - insider shares with both groups the user is already in // - no subshares in this case [ @@ -368,7 +416,7 @@ public function sharesDataProvider() { ] ], [ - // #9 legitimate shares: + // #11 legitimate shares: // - group share with same group // - group share with other group // - user share where recipient renamed @@ -392,7 +440,7 @@ public function sharesDataProvider() { ] ], [ - // #10 legitimate share: + // #12 legitimate share: // - outsider shares with group and user directly with different permissions // - no subshares // - same targets From 7a2d25fab4bb2b452b513e41adda4dec68e81bbe Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 12 Aug 2016 12:06:57 +0200 Subject: [PATCH 2/3] Fix unmerged shares repair with mixed group and direct shares Whenever a group share is created after a direct share, the stime order needs to be properly considered in the repair routine, considering that the direct user share is appended to the $subShares array and breaking its order. --- lib/private/Repair/RepairUnmergedShares.php | 16 +++++-- tests/lib/Repair/RepairUnmergedSharesTest.php | 45 ++++++++++++++++++- 2 files changed, 57 insertions(+), 4 deletions(-) diff --git a/lib/private/Repair/RepairUnmergedShares.php b/lib/private/Repair/RepairUnmergedShares.php index f14e98c17619a..c29de87c4e8a6 100644 --- a/lib/private/Repair/RepairUnmergedShares.php +++ b/lib/private/Repair/RepairUnmergedShares.php @@ -93,7 +93,7 @@ private function buildPreparedQueries() { */ $query = $this->connection->getQueryBuilder(); $query - ->select('item_source', 'id', 'file_target', 'permissions', 'parent', 'share_type') + ->select('item_source', 'id', 'file_target', 'permissions', 'parent', 'share_type', 'stime') ->from('share') ->where($query->expr()->eq('share_type', $query->createParameter('shareType'))) ->andWhere($query->expr()->in('share_with', $query->createParameter('shareWiths'))) @@ -161,13 +161,23 @@ private function getSharesWithUser($shareType, $shareWiths) { * If no suitable subshare is found, use the least recent group share instead. * * @param array $groupShares group share entries - * @param array $subShares sub share entries, sorted by stime + * @param array $subShares sub share entries * * @return string chosen target name */ private function findBestTargetName($groupShares, $subShares) { $pickedShare = null; - // note subShares are sorted by stime from oldest to newest + // sort by stime, this also properly sorts the direct user share if any + @usort($subShares, function($a, $b) { + if ($a['stime'] < $b['stime']) { + return -1; + } else if ($a['stime'] > $b['stime']) { + return 1; + } + + return 0; + }); + foreach ($subShares as $subShare) { // skip entries that have parenthesis with numbers if (preg_match('/\([0-9]*\)/', $subShare['file_target']) === 1) { diff --git a/tests/lib/Repair/RepairUnmergedSharesTest.php b/tests/lib/Repair/RepairUnmergedSharesTest.php index 9fe9a07388102..7304bfd69208f 100644 --- a/tests/lib/Repair/RepairUnmergedSharesTest.php +++ b/tests/lib/Repair/RepairUnmergedSharesTest.php @@ -44,6 +44,9 @@ class RepairUnmergedSharesTest extends TestCase { /** @var \OCP\IDBConnection */ private $connection; + /** @var int */ + private $lastShareTime; + protected function setUp() { parent::setUp(); @@ -92,6 +95,9 @@ protected function setUp() { } })); + // used to generate incremental stimes + $this->lastShareTime = time(); + /** @var \OCP\IConfig $config */ $this->repair = new RepairUnmergedShares($config, $this->connection, $userManager, $groupManager); } @@ -108,6 +114,7 @@ protected function deleteAllShares() { } private function createShare($type, $sourceId, $recipient, $targetName, $permissions, $parentId = null) { + $this->lastShareTime += 100; $qb = $this->connection->getQueryBuilder(); $values = [ 'share_type' => $qb->expr()->literal($type), @@ -119,7 +126,7 @@ private function createShare($type, $sourceId, $recipient, $targetName, $permiss 'file_source' => $qb->expr()->literal($sourceId), 'file_target' => $qb->expr()->literal($targetName), 'permissions' => $qb->expr()->literal($permissions), - 'stime' => $qb->expr()->literal(time()), + 'stime' => $qb->expr()->literal($this->lastShareTime), ]; if ($parentId !== null) { $values['parent'] = $qb->expr()->literal($parentId); @@ -458,6 +465,42 @@ public function sharesDataProvider() { ['/test (4)', 31], ] ], + [ + // #13 bogus share: + // - outsider shares with group1, user2 and then group2 + // - user renamed share as soon as it arrived before the next share (order) + // - one subshare for each group share + // - one extra share entry for direct share to user2 + // - non-matching targets + [ + // first share with group + [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup1', '/test', 31], + // recipient renames + [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/first', 31, 0], + // then direct share, user renames too + [Constants::SHARE_TYPE_USER, 123, 'user2', '/second', 31], + // another share with the second group + [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup2', '/test', 31], + // use renames it + [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/third', 31, 1], + // different unrelated share + [Constants::SHARE_TYPE_GROUP, 456, 'recipientgroup1', '/test (5)', 31], + ], + [ + // group share with group1 left alone + ['/test', 31], + // first subshare repaired + ['/third', 31], + // direct user share repaired + ['/third', 31], + // group share with group2 left alone + ['/test', 31], + // second subshare repaired + ['/third', 31], + // leave unrelated alone + ['/test (5)', 31], + ] + ], ]; } From df9b509ed33ef6e3041b76b4b7ce1b22c7d81fcc Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 17 Aug 2016 09:58:53 +0200 Subject: [PATCH 3/3] Improve regexp to detect duplicate folders when repairing unmerged shares --- lib/private/Repair/RepairUnmergedShares.php | 14 ++- tests/lib/Repair/RepairUnmergedSharesTest.php | 102 ++++++++++++------ 2 files changed, 74 insertions(+), 42 deletions(-) diff --git a/lib/private/Repair/RepairUnmergedShares.php b/lib/private/Repair/RepairUnmergedShares.php index c29de87c4e8a6..d57bc3779f859 100644 --- a/lib/private/Repair/RepairUnmergedShares.php +++ b/lib/private/Repair/RepairUnmergedShares.php @@ -148,6 +148,10 @@ private function getSharesWithUser($shareType, $shareWiths) { return $groupedShares; } + private function isPotentialDuplicateName($name) { + return (preg_match('/\(\d+\)(\.[^\.]+)?$/', $name) === 1); + } + /** * Decide on the best target name based on all group shares and subshares, * goal is to increase the likeliness that the chosen name matches what @@ -169,18 +173,12 @@ private function findBestTargetName($groupShares, $subShares) { $pickedShare = null; // sort by stime, this also properly sorts the direct user share if any @usort($subShares, function($a, $b) { - if ($a['stime'] < $b['stime']) { - return -1; - } else if ($a['stime'] > $b['stime']) { - return 1; - } - - return 0; + return ((int)$a['stime'] - (int)$b['stime']); }); foreach ($subShares as $subShare) { // skip entries that have parenthesis with numbers - if (preg_match('/\([0-9]*\)/', $subShare['file_target']) === 1) { + if ($this->isPotentialDuplicateName($subShare['file_target'])) { continue; } // pick any share found that would match, the last being the most recent diff --git a/tests/lib/Repair/RepairUnmergedSharesTest.php b/tests/lib/Repair/RepairUnmergedSharesTest.php index 7304bfd69208f..7b9d257938900 100644 --- a/tests/lib/Repair/RepairUnmergedSharesTest.php +++ b/tests/lib/Repair/RepairUnmergedSharesTest.php @@ -28,6 +28,8 @@ use OCP\Migration\IRepairStep; use Test\TestCase; use OC\Share20\DefaultShareProvider; +use OCP\IUserManager; +use OCP\IGroupManager; /** * Tests for repairing invalid shares @@ -47,6 +49,12 @@ class RepairUnmergedSharesTest extends TestCase { /** @var int */ private $lastShareTime; + /** @var IUserManager */ + private $userManager; + + /** @var IGroupManager */ + private $groupManager; + protected function setUp() { parent::setUp(); @@ -61,45 +69,14 @@ protected function setUp() { $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); - } - })); + $this->userManager = $this->getMock('\OCP\IUserManager'); + $this->groupManager = $this->getMock('\OCP\IGroupManager'); // used to generate incremental stimes $this->lastShareTime = time(); /** @var \OCP\IConfig $config */ - $this->repair = new RepairUnmergedShares($config, $this->connection, $userManager, $groupManager); + $this->repair = new RepairUnmergedShares($config, $this->connection, $this->userManager, $this->groupManager); } protected function tearDown() { @@ -510,6 +487,38 @@ public function sharesDataProvider() { * @dataProvider sharesDataProvider */ public function testMergeGroupShares($shares, $expectedShares) { + $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]; + + $this->groupManager->expects($this->any()) + ->method('getUserGroupIds') + ->will($this->returnValueMap([ + // owner + [$user1, ['samegroup1', 'samegroup2']], + // recipient + [$user2, ['recipientgroup1', 'recipientgroup2']], + ])); + + $this->userManager->expects($this->once()) + ->method('countUsers') + ->will($this->returnValue([2])); + $this->userManager->expects($this->once()) + ->method('callForAllUsers') + ->will($this->returnCallback(function(\Closure $closure) use ($users) { + foreach ($users as $user) { + $closure($user); + } + })); + $shareIds = []; foreach ($shares as $share) { @@ -536,5 +545,30 @@ public function testMergeGroupShares($shares, $expectedShares) { $this->assertEquals($expectedShare[1], $share['permissions']); } } + + public function duplicateNamesProvider() { + return [ + // matching + ['filename (1).txt', true], + ['folder (2)', true], + ['filename (1)(2).txt', true], + // non-matching + ['filename ().txt', false], + ['folder ()', false], + ['folder (1x)', false], + ['folder (x1)', false], + ['filename (a)', false], + ['filename (1).', false], + ['filename (1).txt.txt', false], + ['filename (1)..txt', false], + ]; + } + + /** + * @dataProvider duplicateNamesProvider + */ + public function testIsPotentialDuplicateName($name, $expectedResult) { + $this->assertEquals($expectedResult, $this->invokePrivate($this->repair, 'isPotentialDuplicateName', [$name])); + } }