From 03a65e5e3a1100d8cbe0a8221c5d127cec30ca4a Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 12 Aug 2016 11:44:34 +0200 Subject: [PATCH 1/2] 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 353877bb8737..f14e98c17619 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 fe9b3e5b96f3..9fe9a0738810 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 8cca3641603d755ed2da6e4be5fc4748f591e9e2 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 12 Aug 2016 12:06:57 +0200 Subject: [PATCH 2/2] 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 f14e98c17619..c29de87c4e8a 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 9fe9a0738810..7304bfd69208 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], + ] + ], ]; }