From 837dc0b33649d0aa6e676601d393f1f5bc354f40 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 353877bb87..f14e98c176 100644 --- a/lib/private/Repair/RepairUnmergedShares.php +++ b/lib/private/Repair/RepairUnmergedShares.php @@ -148,6 +148,44 @@ class RepairUnmergedShares implements IRepairStep { 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 @@ class RepairUnmergedShares implements IRepairStep { 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 fe9b3e5b96..9fe9a07388 100644 --- a/tests/lib/Repair/RepairUnmergedSharesTest.php +++ b/tests/lib/Repair/RepairUnmergedSharesTest.php @@ -204,7 +204,7 @@ class RepairUnmergedSharesTest extends TestCase { [ // #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 @@ class RepairUnmergedSharesTest extends TestCase { [ ['/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 @@ class RepairUnmergedSharesTest extends TestCase { [ // #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 @@ class RepairUnmergedSharesTest extends TestCase { ] ], [ - // #4 bogus share: + // #6 bogus share: // - outsider shares with group1, group2 // - one subshare for each group share // - non-matching targets @@ -276,7 +324,7 @@ class RepairUnmergedSharesTest extends TestCase { ] ], [ - // #5 bogus share: + // #7 bogus share: // - outsider shares with group1, group2 // - one subshare for each group share // - non-matching targets @@ -301,7 +349,7 @@ class RepairUnmergedSharesTest extends TestCase { ] ], [ - // #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 @@ class RepairUnmergedSharesTest extends TestCase { ] ], [ - // #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 @@ class RepairUnmergedSharesTest extends TestCase { ] ], [ - // #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 @@ class RepairUnmergedSharesTest extends TestCase { ] ], [ - // #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 @@ class RepairUnmergedSharesTest extends TestCase { ] ], [ - // #10 legitimate share: + // #12 legitimate share: // - outsider shares with group and user directly with different permissions // - no subshares // - same targets From c1ad88a7774ac6d10b00a0a80c3d25b601a418a1 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 f14e98c176..c29de87c4e 100644 --- a/lib/private/Repair/RepairUnmergedShares.php +++ b/lib/private/Repair/RepairUnmergedShares.php @@ -93,7 +93,7 @@ class RepairUnmergedShares implements IRepairStep { */ $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 @@ class RepairUnmergedShares implements IRepairStep { * 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 9fe9a07388..7304bfd692 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 @@ class RepairUnmergedSharesTest extends TestCase { } })); + // 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 @@ class RepairUnmergedSharesTest extends TestCase { } 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 @@ class RepairUnmergedSharesTest extends TestCase { '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 @@ class RepairUnmergedSharesTest extends TestCase { ['/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 11e9ce669a67981031bf2bc32c467c666b9e9aa9 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 c29de87c4e..d57bc3779f 100644 --- a/lib/private/Repair/RepairUnmergedShares.php +++ b/lib/private/Repair/RepairUnmergedShares.php @@ -148,6 +148,10 @@ class RepairUnmergedShares implements IRepairStep { 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 @@ class RepairUnmergedShares implements IRepairStep { $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 7304bfd692..7b9d257938 100644 --- a/tests/lib/Repair/RepairUnmergedSharesTest.php +++ b/tests/lib/Repair/RepairUnmergedSharesTest.php @@ -28,6 +28,8 @@ use OCP\Migration\IOutput; 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 @@ class RepairUnmergedSharesTest extends TestCase { $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 @@ class RepairUnmergedSharesTest extends TestCase { * @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 @@ class RepairUnmergedSharesTest extends TestCase { $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])); + } }