diff --git a/lib/private/Repair/RepairUnmergedShares.php b/lib/private/Repair/RepairUnmergedShares.php index 353877bb8737d..d57bc3779f859 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'))) @@ -148,6 +148,52 @@ 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 + * 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 + * + * @return string chosen target name + */ + 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) { + return ((int)$a['stime'] - (int)$b['stime']); + }); + + foreach ($subShares as $subShare) { + // skip entries that have parenthesis with numbers + if ($this->isPotentialDuplicateName($subShare['file_target'])) { + 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 +217,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..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 @@ -44,6 +46,15 @@ class RepairUnmergedSharesTest extends TestCase { /** @var \OCP\IDBConnection */ private $connection; + /** @var int */ + private $lastShareTime; + + /** @var IUserManager */ + private $userManager; + + /** @var IGroupManager */ + private $groupManager; + protected function setUp() { parent::setUp(); @@ -58,42 +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']], - ])); + $this->userManager = $this->getMock('\OCP\IUserManager'); + $this->groupManager = $this->getMock('\OCP\IGroupManager'); - $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); - } - })); + // 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() { @@ -108,6 +91,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 +103,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); @@ -204,7 +188,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 +202,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 +212,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 +283,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 +308,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 +333,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 +361,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 +382,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 +400,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 +424,7 @@ public function sharesDataProvider() { ] ], [ - // #10 legitimate share: + // #12 legitimate share: // - outsider shares with group and user directly with different permissions // - no subshares // - same targets @@ -410,6 +442,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], + ] + ], ]; } @@ -419,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) { @@ -445,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])); + } }