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.

diff --git a/lib/private/Repair/RepairUnmergedShares.php b/lib/private/Repair/RepairUnmergedShares.php
index f14e98c17619abcd4e3b6789cf5773f70351e415..c29de87c4e8a6258a3729cb4ff1dbfce3fab21a5 100644 (file)
--- 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 9fe9a07388102a8c94670047d028fba1ce1c5038..7304bfd69208fd6b105a98afa3aae9710b30783d 100644 (file)
--- 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],
+                               ]
+                       ],
                ];
        }